- Notifications
You must be signed in to change notification settings - Fork328
make session backwards compatible#1528
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
23b243b intomasterUh oh!
There was an error while loading.Please reload this page.
| // Get the notification that triggered this call. | ||
| //Guaranteed to existsinceit built the component that calledthis, so thisissafe to unwrap. | ||
| // Get the notification that triggered this call.. | ||
| //unwrap notifications if finesincewe should panic ifthisismissing. |
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.
We should not be panicking if anything optional like this is missing. We should default tovec![].
| ]; | ||
| let response = client | ||
| .get("/notifications/product/modal/remove_modal?id=1") |
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.
This feels like the wrong place to implement this. This should be in our web app, and we should pass it through the context into the dashboard.
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.
Not sure what you mean here. The test, or the endpoint should be in web app? I felt like both are needed here since notifications include marketing notifications.
This reverts commit23b243b.
Uh oh!
There was an error while loading.Please reload this page.