-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Push new notice on recent phishing incidents #4566
Conversation
Looks like our integration tests are currently opinionated about the number of notifications shown. @tmashuang do you think you could make these tests more general? |
currently the phishing warning is coming BEFORE normal startup notices like the privacy notice |
That’s strange. Notices should probably appear chronologically. |
picking this up |
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.
Looks good. Much easier to manage notices this way. Assuming tests now pass, good to merge.
nah tests are not dynamic yet |
i think we should remove ToS and privacy from notices, and move notices to a more in-app notification thing. then we can also move notices to something we can deploy live out of scope of this PR, needs design, yada yada |
for now hard coding the phishing warning in integration tests |
ok heres my last attempt for tonight |
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.
LGTM, we’ll QA on develop.
I need someone to manually QA this before we merge--I am unable to get my builds working locally.
This adds a new notice to our notice controller, allowing us to notify all of our users on next update of the recent phishing concerns.