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.

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

[Reports] Disable GO button once clicked #12619

Merged

Conversation

rioug
Copy link
Collaborator

@rioug rioug commented Jun 26, 2024

What? Why?

The main issue was that report where slow to generate, but because we did not disable the Go button, users were able to spam the server adding unnecessary to load. This PR address disabling the button to prevent such spamming.

The button is disabled client side and server side, so even if the server is slow to respond or the websocket connection drops the button will still be disabled. It will be re enabled once the report is rendered.

What should we test?

As an enterprise user

  • visit reports tab
  • Choose any report, preferably one that you know has the potential to be slow
  • Generate a report by clicking "Go"
    -> Notice the button being disabled *, and clicking doesn't do anything
    -> Notice the button being enabled ** again

*

  • With admin v3 enabled : button becomes grey, and pointer doesn't change
  • With legacy style : the button doesn't change but clicking doesn't do anything

**

  • With admin v3 enabled : button becomes blue again, and pointer change to a hand
  • With legacy style : the button doesn't change but clicking should load the report 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

It is disabled both on client side and server side, so even if the
server takes a while to respond the button is disabled
@rioug rioug added the user facing changes Thes pull requests affect the user experience label Jun 26, 2024
@sigmundpetersen sigmundpetersen changed the title [Report ]Disable GO button once clicked [Report] Disable GO button once clicked Jun 26, 2024
@sigmundpetersen sigmundpetersen changed the title [Report] Disable GO button once clicked [Reports] Disable GO button once clicked Jun 26, 2024
Copy link
Collaborator

@chahmedejaz chahmedejaz left a comment

Choose a reason for hiding this comment

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

Nicely done 👍

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great, thanks I think this could make a big difference.

I think we could simplify it, but this is good 👍

app/controllers/admin/reports_controller.rb Show resolved Hide resolved
Comment on lines +64 to +65
selector: "#report-go",
html: helpers.button(t(:go), "report__submit-btn", "submit", disabled: true)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this is actually needed because we do it client-side. But it doesn't hurt either.

Comment on lines 7 to 8
go_button = find("button.report__submit-btn")
expect(go_button).to be_disabled
Copy link
Member

Choose a reason for hiding this comment

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

Can we do something like expect(page).to have_button "Go", disabled: true?

Copy link
Collaborator

@chahmedejaz chahmedejaz left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@drummer83 drummer83 self-assigned this Jun 28, 2024
@drummer83 drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Jun 28, 2024
@drummer83
Copy link
Contributor

Hi @rioug,
Thanks for improving our reports and the excellent test instructions! 🙏

I have tested this and I can confirm:

  • After clicking GO for the first time, additional clicks will not trigger another report (I checked the browser console to validate that). ✔️
  • Admin_v3: The button turns gray, can't be clicked again and the mouse cursor doesn't change on hover. ✔️
  • Legacy: The button and mouse don't change, but clicking doesn't do anything. ✔️
  • After the report has been generated, the button resumes to the original state and can be clicked again. ✔️

I noticed one little thing, which isn't a blocker. Immediately after clicking, the label of the button turns from GO to TRUE. It returns to GO very quickly after. Is it possible to change that?
Here is what I mean:

Kazam_screencast_00002.webm
Kazam_screencast_00003.webm

Anyway, I did not notice any regression, so this one is ready to go! 🥳
Thanks again! 🚀

@drummer83 drummer83 merged commit 5bb4782 into openfoodfoundation:master Jun 28, 2024
52 checks passed
@drummer83 drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Jun 28, 2024
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

@rioug I think this is a quick fix, would you be able to try it out?

.actions.filter-actions
= button t(:go), "report__submit-btn"
#report-go.actions.filter-actions{ data: { controller: "scoped-channel", "scoped-channel-id-value": request.uuid } }
= button t(:go), "report__submit-btn", "submit", "data-disable-with": true
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't pick up on this before. The data-disable-with attribute is used to replace the button text.
Usually, rails magic JS will auto-populate this, but I guess you found that you had to manually include it.

I think we can simply include the go text here:

Suggested change
= button t(:go), "report__submit-btn", "submit", "data-disable-with": true
= button t(:go), "report__submit-btn", "submit", "data-disable-with": t(:go)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

look like I miss read the documentation it should have been :

Suggested change
= button t(:go), "report__submit-btn", "submit", "data-disable-with": true
= button t(:go), "report__submit-btn", "submit", "data-disable": true

I'll fix that quickly

@rioug
Copy link
Collaborator Author

rioug commented Jul 1, 2024

@rioug I think this is a quick fix, would you be able to try it out?

Fixed here #12628

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants