Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUU] Remove Stimulus Reflex from Products screen #12328

Merged
merged 13 commits into from
Apr 10, 2024
Merged

[BUU] Remove Stimulus Reflex from Products screen #12328

merged 13 commits into from
Apr 10, 2024

Conversation

dacook
Copy link
Member

@dacook dacook commented Apr 2, 2024 •
edited

What? Why?

After much discussion, I have removed most of the StimulusReflex from the new Products screen. The plan is to make it more dynamic with Turbo/AJAX.

SR is still there for deleting records, which could still suffer the loading issues we had. (It's also used for the Edit Image modal, but this shouldn't have the loading issue).
I expect to continue with removing those, but it would be more efficient to transfer them directly to a Turbo/AJAX implementation later.

What should we test?

All the things!! 馃珷 (almost)

  • Page load: shows products as expected
  • Filtering: by search term, products, category
  • Pagination:
    • choose number per page
    • browse pages
  • Submit changes
    • PRoducts
    • Variants
    • Form errors (eg blank product name)
  • New variant
    • Form errors (eg blank unit)
  • Product actions work as before
    • Edit image (still uses a little bit of StimulusReflex)
    • Edit record
    • Delete (still uses StimulusReflex to load products after)
    • Clone
  • Variant actions: these work the same as product, so no need to test again.

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

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

rioug
rioug reviewed Apr 2, 2024
View reviewed changes
config/routes/admin.rb Outdated Show resolved Hide resolved
@dacook dacook force-pushed the buu/remove-stimulus-reflex branch 3 times, most recently from 1fcd7c7 to 65efb8c Compare April 4, 2024 05:23
@dacook dacook marked this pull request as ready for review April 4, 2024 05:23
@dacook dacook requested a review from mkllnk April 4, 2024 05:24
@dacook dacook self-assigned this Apr 4, 2024
@dacook dacook added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Apr 4, 2024
mkllnk
mkllnk approved these changes Apr 4, 2024
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did only a quick review but didn't find anything obviously wrong. It looks easier than I thought to remove SR from the page.

@dacook dacook mentioned this pull request Apr 4, 2024
Merged
rioug
rioug approved these changes Apr 7, 2024
View reviewed changes
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine !

@dacook dacook force-pushed the buu/remove-stimulus-reflex branch from 5c7ff95 to eebb25f Compare April 9, 2024 07:20
@dacook
Copy link
Member Author

dacook commented Apr 9, 2024

Sorry @filipefurtad0 I seem to have missed something in the rebase, I'll have to fix it up for testing tomorrow instead.

@dacook dacook force-pushed the buu/remove-stimulus-reflex branch from d72eb78 to 4b5f169 Compare April 10, 2024 04:54
@dacook
Copy link
Member Author

dacook commented Apr 10, 2024

@filipefurtad0 I've fixed the rebase, and added testing notes. Please test away! 馃И

@filipefurtad0 filipefurtad0 self-assigned this Apr 10, 2024
@filipefurtad0 filipefurtad0 added the no-staging-UK A tag which does not trigger deployments, indicating a server is being used label Apr 10, 2024
@dacook @filipefurtad0
Don't warn when submitting form
d904c2a
@dacook @filipefurtad0
Prettify
dadabcf
@dacook @filipefurtad0
Move index and bulk_update actions to good ol' HTTP requests
8696e05 
We've found that we just can't rely in StimulusReflex (and the underlying WebSockets stack) to guarantee a response to a request.
Because of this, there was intermittent issues when the server was overloaded with large requests, and the response never arrived, leaving an infinite loader, and a poor user wondering if anything was still happening.
@dacook @filipefurtad0
Discard changes by reloading the page
2c71f7f 
Now there's a popup asking to confirm, which I think is worth keeping!
@dacook @filipefurtad0
Ensure pagination is retained when saving or discarding
0fc3d39 
But we have more work to do.
@dacook @filipefurtad0
Use shared page controls on products screen
fd8be37 
This has an auto submit and can potentially work with Turbo, like on the Orders screen.
@dacook @filipefurtad0
Avoid pagination when editing errored products
c4b7b76
@dacook @filipefurtad0
Prevent duplicate products in search results
54d0dfb 
Dunno why, but the product was appearing once for each variant.
@dacook @filipefurtad0
Only register event listeners when needed
15790d3 
Otherwise there could be over 200 listeners on a page.
@dacook @filipefurtad0
Fix intermittent bug
97d1359 
Dunno why, but this recently started occuring for me in dev and test. Browser update?
@dacook @filipefurtad0
Fix spec
129ceb0
@dacook @filipefurtad0
TODO
e78ef12 
I think this case got lost.
@dacook @filipefurtad0
Fix product filtering
1425d52 
Merges change from fb09a7f
@filipefurtad0 filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed no-staging-UK A tag which does not trigger deployments, indicating a server is being used pr-staged-fr staging.coopcircuits.fr labels Apr 10, 2024
@filipefurtad0
Copy link
Contributor

Hi @dacook ,

Unfortunately, I just lost all my notes, but the summary is - let's merge.

I went through all the test you indicate (thank you 馃檹) and spotted only a minor regression, when editing a product/variant, with any given filter:

  • before this PR, after saving changes, the filters were kept (only a subset of variants is displayed, accordingly)
  • after this PR, after saving changes, the filters were cleared (all available variants are displayed)

Not related to this PR
I found two usability issues as well:

  • click "edit variant"; instead of editing, click "cancel" -> redirect should occur to the products page, but instead, redirects to the page of that product (perhaps per-existent, and not a priority)
  • editing "Units" from a variant; delete the existing value; change your mind and wish to do something else -> you can't click away from the edit unit sub menu, neither Esc or clicking elsewhere works. That means you need to input a value. Only workaround is refreshing the page.

Also, I've merged this branch on top of the specs for BUU and spotted no additional failing specs.

Merging!

@filipefurtad0 filipefurtad0 merged commit 8903357 into openfoodfoundation:master Apr 10, 2024
52 checks passed
@dacook
Copy link
Member Author

dacook commented Apr 11, 2024

Hi Filipe, thanks for testing this one!

I went through all the test you indicate (thank you 馃檹) and spotted only a minor regression, when editing a product/variant, with any given filter:

  • before this PR, after saving changes, the filters were kept (only a subset of variants is displayed, accordingly)
  • after this PR, after saving changes, the filters were cleared (all available variants are displayed)

Good spotting, noted. I've fixed this in my next branch.

Not related to this PR I found two usability issues as well:

I thought it would be a good opportunity to test out the general usability of the page ;)

  • click "edit variant"; instead of editing, click "cancel" -> redirect should occur to the products page, but instead, redirects to the page of that product (perhaps per-existent, and not a priority)
  • editing "Units" from a variant; delete the existing value; change your mind and wish to do something else -> you can't click away from the edit unit sub menu, neither Esc or clicking elsewhere works. That means you need to input a value. Only workaround is refreshing the page.

I can see how this is a bit annoying, but I found it usable: I was able to click "discard changes" to reset, or press CTRL/CMD-Z to undo. Or just type it in again ;)

However there is a problem with units when creating a new variant.

Also, I've merged this branch on top of the specs for BUU and spotted no additional failing specs.

馃帀

@chahmedejaz chahmedejaz mentioned this pull request Apr 14, 2024
Open
3 tasks
dacook added a commit to dacook/openfoodnetwork that referenced this pull request Apr 23, 2024
@dacook
[tofix] Add ability to bulk update products for product managers
e6767c4 
I forgot to do this in openfoodfoundation#12328 [BUU] Remove Stimulus Reflex from Products screen
dacook added a commit to dacook/openfoodnetwork that referenced this pull request Apr 23, 2024
@dacook
Add ability to bulk update products for product managers
9b9e459 
I forgot to do this in openfoodfoundation#12328 [BUU] Remove Stimulus Reflex from Products screen
dacook added a commit to dacook/openfoodnetwork that referenced this pull request Apr 24, 2024
@dacook
Add ability to bulk update products for product managers
b846d0f 
I forgot to do this in openfoodfoundation#12328 [BUU] Remove Stimulus Reflex from Products screen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkllnk mkllnk mkllnk approved these changes

@rioug rioug rioug approved these changes

Assignees

@dacook dacook

@filipefurtad0 filipefurtad0

Labels
feature toggled These pull requests' changes are invisible by default and are grouped in release notes
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUU] [Infinite loader] - Remove Stimulus Reflex from Products Page
4 participants
@dacook @filipefurtad0 @mkllnk @rioug