-
Notifications
You must be signed in to change notification settings - Fork 1
feat: updated notification preferences unsubscribe flow #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: updated notification preferences unsubscribe flow #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the notification preferences unsubscribe flow by removing the requirement for a hashed patch parameter while maintaining backwards compatibility. The change simplifies the URL structure by making the updatePatch
parameter optional.
- Removed
updatePatch
parameter from API functions and component logic - Updated route definition to make
updatePatch
optional for backwards compatibility - Modified tests to reflect the simplified parameter structure
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/preferences-unsubscribe/index.test.jsx | Updated test setup to remove updatePatch parameter and simplified test assertions |
src/preferences-unsubscribe/index.jsx | Removed updatePatch from useParams and API calls, simplified tracking event |
src/preferences-unsubscribe/data/api.js | Simplified API functions to only require userToken parameter |
src/constants.ts | Made updatePatch parameter optional in route definition for backwards compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
() => setStatus(LOADED), | ||
(error) => { | ||
setStatus(FAILED); | ||
logError(error); | ||
}, | ||
); | ||
sendTrackEvent('edx.ui.lms.notifications.preferences.unsubscribe', { userToken, updatePatch }); | ||
sendTrackEvent('edx.ui.lms.notifications.preferences.unsubscribe', { userToken }); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect dependency array is missing the userToken
dependency. Since userToken
is used inside the effect, it should be included in the dependency array to ensure the effect re-runs when the token changes.
}, []); | |
}, [userToken]); |
Copilot uses AI. Check for mistakes.
86921d0
to
c21e7d6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9 +/- ##
=========================================
Coverage ? 90.90%
=========================================
Files ? 344
Lines ? 5784
Branches ? 1369
=========================================
Hits ? 5258
Misses ? 507
Partials ? 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Updated notification unsubscribe flow url