[BUU] Fix Messy flash notifications on new products page #12778

chahmedejaz · 2024-08-15T00:07:34Z

What? Why?

Closes [BUU] Messy flash notifications on new products page #12596
If the distributors have active order cycles but haven't set up shipping or payment methods, then a warning is displayed on the new screen.
This warning is displayed each time the user visits any screen or switches tabs/pages.
This PR fixes this such that the warning is displayed only once per user session.

What should we test?

Have distributors that don't have payment or shipping methods setup
Have order cycles opened for them
Once you login, on the new admin screen, you should be able to see the order cycle warning
Refresh the page, switch the tab, or navigate to another page, the order cycle warning should not be displayed anymore

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

chahmedejaz · 2024-08-15T00:26:46Z

app/controllers/spree/admin/base_controller.rb

+
+        html_request?
+      end
+
      def html_request?


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:

https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/controllers/admin/customers_controller.rb#L8

- 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

mkllnk · 2024-08-15T01:30:17Z

app/controllers/spree/admin/base_controller.rb

+        return if warning.blank?
+
+        flash.now[:notice] = warning
+        session[:displayed_order_cycle_warning] = true


Does this mean that we display the message only once?

Yes, only once during the user's login session. If he re-logins then the warning will again pop up.

Ref: https://openfoodnetwork.slack.com/archives/C01CXQNJ1J6/p1723641747029789?thread_ts=1723641747.029789&cid=C01CXQNJ1J6

dacook

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:

dacook · 2024-08-15T02:10:41Z

app/controllers/spree/admin/base_controller.rb


      # 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?


± I think we should still keep this condition. It ensure we don't overwrite a more specific notice.

change requested.

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

Ah ha, thanks for explaining. So this case should never occur.
Oh well, it doesn't hurt to be defensive here.

app/controllers/spree/admin/base_controller.rb

dacook · 2024-08-15T02:31:07Z

app/controllers/spree/admin/base_controller.rb


      # 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?


change requested.

chahmedejaz · 2024-08-15T10:13:33Z

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).

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 😅

It might be safer to wait until the user manually dismisses it.

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.

mkllnk

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.

dacook

Great, ready to test.

dacook · 2024-08-16T00:29:05Z

I think this is pretty important for the BUU rollout, so I'm testing it now for inclusion.

Staged on uk_staging. I can see the notice on first load:

✅ On subsequent load, in any tab, it no longer appears.
✅ After logout > login > Admin: it appears again.

chahmedejaz · 2024-08-16T05:30:24Z

Thank you so much @dacook ♥️🎉

github-actions bot assigned chahmedejaz Aug 15, 2024

chahmedejaz added bug-s4 The bug is annoying, but doesn't prevent from using the platform. Not so many users are impacted. user facing changes Thes pull requests affect the user experience labels Aug 15, 2024

chahmedejaz force-pushed the bugfix/12596-fix-annoying-oc-warning-display branch from 2e09952 to 9318916 Compare August 15, 2024 00:23

chahmedejaz commented Aug 15, 2024

View reviewed changes

chahmedejaz added 2 commits August 15, 2024 05:59

12596 - fix annoying oc warning display

3474c60

- such that it only displays once per user session

12596 - fix specs

282df98

- 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

chahmedejaz force-pushed the bugfix/12596-fix-annoying-oc-warning-display branch from 4700508 to 282df98 Compare August 15, 2024 01:00

chahmedejaz marked this pull request as ready for review August 15, 2024 01:12

mkllnk reviewed Aug 15, 2024

View reviewed changes

chahmedejaz requested a review from mkllnk August 15, 2024 01:36

dacook approved these changes Aug 15, 2024

View reviewed changes

dacook requested changes Aug 15, 2024

View reviewed changes

dacook mentioned this pull request Aug 15, 2024

Release v4.6 #12765

Closed

12 tasks

12596: keep flash[:notice] check

46e54f4

chahmedejaz requested a review from dacook August 15, 2024 10:21

mkllnk approved these changes Aug 15, 2024

View reviewed changes

dacook approved these changes Aug 16, 2024

View reviewed changes

dacook added the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 16, 2024

dacook removed the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 16, 2024

dacook merged commit 9170799 into openfoodfoundation:master Aug 16, 2024
52 checks passed

chahmedejaz mentioned this pull request Aug 16, 2024

Do not check missing ship/pay methods on turbo request #12672

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUU] Fix Messy flash notifications on new products page #12778

[BUU] Fix Messy flash notifications on new products page #12778

chahmedejaz commented Aug 15, 2024 •

edited

Loading

chahmedejaz Aug 15, 2024

mkllnk Aug 15, 2024

chahmedejaz Aug 15, 2024

chahmedejaz Aug 15, 2024

dacook left a comment •

edited

Loading

dacook Aug 15, 2024 •

edited

Loading

dacook Aug 15, 2024

chahmedejaz Aug 15, 2024 •

edited

Loading

dacook Aug 16, 2024

dacook Aug 15, 2024

chahmedejaz commented Aug 15, 2024 •

edited

Loading

mkllnk left a comment

dacook left a comment

dacook commented Aug 16, 2024

chahmedejaz commented Aug 16, 2024

[BUU] Fix Messy flash notifications on new products page #12778

[BUU] Fix Messy flash notifications on new products page #12778

Conversation

chahmedejaz commented Aug 15, 2024 • edited Loading

What? Why?

What should we test?

Release notes

chahmedejaz Aug 15, 2024

Choose a reason for hiding this comment

mkllnk Aug 15, 2024

Choose a reason for hiding this comment

chahmedejaz Aug 15, 2024

Choose a reason for hiding this comment

chahmedejaz Aug 15, 2024

Choose a reason for hiding this comment

dacook left a comment • edited Loading

Choose a reason for hiding this comment

dacook Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

dacook Aug 15, 2024

Choose a reason for hiding this comment

chahmedejaz Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

dacook Aug 16, 2024

Choose a reason for hiding this comment

dacook Aug 15, 2024

Choose a reason for hiding this comment

chahmedejaz commented Aug 15, 2024 • edited Loading

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook commented Aug 16, 2024

chahmedejaz commented Aug 16, 2024

chahmedejaz commented Aug 15, 2024 •

edited

Loading

dacook left a comment •

edited

Loading

dacook Aug 15, 2024 •

edited

Loading

chahmedejaz Aug 15, 2024 •

edited

Loading

chahmedejaz commented Aug 15, 2024 •

edited

Loading