Fix autosave/dialog flow #12521
Merged
Fix autosave/dialog flow #12521
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Plugin builds for f99d42d are ready 🛎️!
|
|
Size Change: +100 B (0%) Total Size: 2.71 MB
ℹ️ View Unchanged
|
swissspidy
approved these changes
Oct 24, 2022
|
Overall, I feel this ticket can be merged with the understanding that there are other autosave-backup-takeover scenarios that need to be ironed out. I tried the following scenarios to verify this PR:
The issue that @miina pointed out where there is no autosave happening after the first one on a published story still exists. This means changes made after take over are not visible on a re-takeover !! But this is a corner case. There are other issues with the interaction between autosave and session-backup that need to be address. For example: Here, even after autosave occurred, there is a backup present and it results in two different prompts to the user - one for restoring from backup and another from autosave !! But this is happening on |
|
@merapi @swissspidy -- I can confirm that the 2nd auto-save triggers after making further changes to the webstory in draft. It happens within 60 sec of the last edit. |
|
These are the sanity workflows that I can confirm work:
|
- Preload first post lock API response so that it’s immediately available in the editor - Immediately fetch information on mount, before interval is up - Fix `act` warnings in tests
Comment on lines
91
to
95
| "/web-stories/v1/$stories_rest_base/{$post->ID}/lock/?" . build_query( | ||
| [ | ||
| '_embed' => 'author', | ||
| ] | ||
| ), |
There was a problem hiding this comment.
This was not preloaded by design. Why is it being preloaded now?
There was a problem hiding this comment.
See discussion at https://xwp.slack.com/archives/C01H2K83WD9/p1666788696201389 for an explanation.
The aim is to immediately get the current lock owner information right when opening the editor, without having to wait for the 60s interval to pass first. There's no harm in preloading the response and saving 1 HTTP request.
Note that preloading a response only stores it once. Any subsequent calls will trigger an HTTP request as expected. Just the first one is fetched from cache.
spacedmonkey
suggested changes
Oct 26, 2022
There was a problem hiding this comment.
Seems like now, I am not seeing the message that says, the story has been taken over. Now, I see, when a story is taken over, I always see this message.
So this is how it should work.
- User 1, edits story. Sets lock.
- User 2, open story.
- User 2, takes over story.
- User 1, see message that story was taken over, without option to over story back over.
Screen.Recording.2022-10-26.at.16.26.18.movSee is now a tag of war between the two users. This is not how it used to be. |
This reverts commit 3e6f6c5.
Reverted the breaking change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR will fix autosave triggered on every element update and 2 issues from here.
There is one more issue with autosave but I need to think about it a little more, it can have a bigger impact on the app (
hasNewChangesis used in a few places).Relevant Technical Choices
To-do
User-facing changes
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #12474