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

Fix action button label changing behavior in MergeUI #8788

Merged

Conversation

mheiman
Copy link
Collaborator

@mheiman mheiman commented Feb 2, 2024

Closes #8655

The MergeUI action button would confusingly change back to "Merge" from "Saving..." or "Done" in the midst of a merge. This PR fixes that behavior.

Technical

The MergeUI interface is designed to change the action button to "Merge" (or "Request Merge") button as soon as enough data has loaded for the merge to complete successfully. This means that when the server is slow, the Merge button will be available before covers, list counts, reading log counts, etc. have loaded. That's the expected behavior.

A previous PR had added a watch function that set the button to "Merge" whenever the internal merge() function returned changed data. The merge() function returns values as soon as a merge is possible, but the returned values change as additional data comes in, so if the user pressed the "Merge" button before e.g. list counts had loaded, and the list counts loaded before the merge was complete, the button would switch from "Saving..." back to "Merge" briefly and then to "Done".

If some of the nonessential data loaded after the merge was complete, the same check would change the button from "Done" back to "Merge".

This PR modifies the behavior of that watch function so that it only changes the button to "Merge" once — when the merge() function first returns data.

Testing

This one is very tricky to test. I had to set up a network debugging tool that would delay lists.api requests by several seconds so that I could reproduce the problem consistently. I solemnly swear that it's fully tested and working. :)

Stakeholders

@mheiman @jimchamp @cdrini @tfmorris @Eds-Dbug

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e570f0d) 16.62% compared to head (94da56e) 16.62%.
Report is 62 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8788   +/-   ##
=======================================
  Coverage   16.62%   16.62%           
=======================================
  Files          88       88           
  Lines        4698     4698           
  Branches      837      838    +1     
=======================================
  Hits          781      781           
  Misses       3399     3399           
  Partials      518      518           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mekarpeles mekarpeles self-assigned this Feb 5, 2024
@mekarpeles mekarpeles merged commit 8c53e59 into internetarchive:master Feb 5, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge button loads before rest of the "Merge Link" page.
3 participants