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

Ensure changes get properly cleared as succeeded and make refresh blocking more targeted #3789

Merged
merged 3 commits into from
Nov 3, 2022

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Nov 2, 2022

Summary

Description of the change(s) you made

  • Reverts previous changes to max rev calculation logic, and instead explicitly polls the backend for server_revs that are marked as synced but not applied, errored, or disallowed in the frontend
  • Makes the blocking of collection refreshes for ContentNodes more targeted, to prevent changes only to tree structure for local nodes (i.e. for fetches for parents or ids queries).

Manual verification steps performed

  1. Add a time.sleep to the _clone_node method of the CustomContentNodeTreeManager to make copies run slowly to replicate the error from production
  2. Copy two folders in the same import from channels modal, to ensure two copy operations are started
  3. When the first copy operation has finished, check that you can click into the folder and that the contents load
  4. When the second copy operation has finished, check that you can click into that folder and that the contents load

Screenshots (if applicable)

Screencast.from.11-02-2022.02.54.48.PM.webm.mp4

Does this introduce any tech-debt items?

I think this can be done much more neatly and efficiently with the websockets work!


Reviewer guidance

How can a reviewer test these changes?

Do the manual steps suggested above.
Review the new logic for more targeted refresh blocking - does this miss any cases?

References

Fixes #3753

Do explicit checks for unapplied revs.
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

I tested moving items to trash and copying multiple folders as described, and it all looks good. Requested changes are:

  • updating params.ids to params.id__in as mentioned
  • perhaps adding || params.parent === c.key-- creating a new folder and immediately opening it is failing

@rtibbles
Copy link
Member Author

rtibbles commented Nov 3, 2022

Will update, thanks for the tests and the eyes on code!

@rtibbles
Copy link
Member Author

rtibbles commented Nov 3, 2022

Updated!

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Changes look good and testing too. One thing I noticed, and can add a followup issue for, is that trying to upload files to a new folder not yet created on the backend ends up failing silently. Looks like it's making a request to fetch the folder content node. We probably want to put some similar checks in the Resource.get method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants