-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
[BUU] Fix Messy flash notifications on new products page #12778
[BUU] Fix Messy flash notifications on new products page #12778
Conversation
2e09952
to
9318916
Compare
|
||
html_request? | ||
end | ||
|
||
def html_request? |
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.
Keeping this helper because there might be some cases where we would want to check for requests that display data (HTML, turbo, etc) rather than serve data (JSON, XML, etc).
Moreover, it's being used here:
- such that it only displays once per user session
- As we are only showing the oc warning once now we need these steps where we are dismissing the oc warning after each navigation. Just keeping the first notice dismiss after login
4700508
to
282df98
Compare
return if warning.blank? | ||
|
||
flash.now[:notice] = warning | ||
session[:displayed_order_cycle_warning] = true |
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.
Does this mean that we display the message only once?
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.
Yes, only once during the user's login session. If he re-logins then the warning will again pop 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.
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.
Great!
There's two changes here:
- Don't check for the warning when making a partial page load (turbo).
This is a good safeguard to avoid doubling it up unnecessarily. But the next change means it wouldn't double anyway...
- The session remembers if you've viewed the warning since last login.
Hmm, how long do sessions last? Apparently we have no limit set, as long as the browser session remains open (which I suspect could be a long time if people leave their computer on).
So it's possible the reminder won't be persistent enough. It might be safer to wait until the user manually dismisses it. But this wasn't specified in the issue, and besides, I think people will be so glad not to see the message!
👍 Ok, thumbs up!
Edit: I forgot to change my review, I think there's one thing to change:
|
||
# Warn the user when they have an active order cycle with hubs that are not ready | ||
# for checkout (ie. does not have valid shipping and payment methods). | ||
def warn_invalid_order_cycles | ||
return if flash[:notice].present? |
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.
±
I think we should still keep this condition. It ensure we don't overwrite a more specific notice.
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.
change requested.
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.
I've addressed this change, however, I tested the scenario where there's a specific flash notice in controller action (which is the case in our app right now), then rails automatically display that and doesn't get overridden by this method because its being called before calling any action (in before_action callback). And every other notices are set in controller actions. So, that controller message would get the priority over this.
This check was present here because before flash[:notice]
was being used, now it's flash.now[:notice]
. This notice was not discarded after being displayed before and we still had that notice for the very next request. The check ensured that if we still had it, then no need to recalculate the notice.
Just in case I'm missing something here, pushing the requested change.
46e54f4
Thanks
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.
Ah ha, thanks for explaining. So this case should never occur.
Oh well, it doesn't hurt to be defensive here.
|
||
# Warn the user when they have an active order cycle with hubs that are not ready | ||
# for checkout (ie. does not have valid shipping and payment methods). | ||
def warn_invalid_order_cycles | ||
return if flash[:notice].present? |
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.
change requested.
Thanks @dacook - Yes, that's concerning. I'm not sure if we should put a limit on session, but looking at from security standpoint, it seems like we should. But in case of UX, I don't think it would be a good idea to have user login much often 😅
Yes, I thought the same. But then I looked into the code, I don't think we have any behavior like this as of now. I doesn't look hard enough, by using user's local storage, cookies and some checks. It seems like a doable task. We should have this behavior for important notices, maybe some feature for the future? Thanks. |
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.
Nice!
I didn't know about the Slack conversation. That's all good then. We may implement something more clever where the user can dismiss the modal and that's stored. But for now this is a good improvement.
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.
Great, ready to test.
Thank you so much @dacook |
What? Why?
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):