Prevent autosave when the post is locked #32475
Conversation
desaiuditd
added
[Type] Bug
|
Some of the changes I've made here ... they can be questionable. Would like to get any initial feedback and iterate on this, if needed. Please feel free to point me to right direction if I've missed or unknowingly neglected anything. |
|
Size Change: +361 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
| @@ -1128,7 +1132,7 @@ export function getPermalinkParts( state ) { | |||
| * @return {boolean} Is locked. | |||
| */ | |||
| export function isPostLocked( state ) { | |||
| return state.postLock.isLocked; | |||
| return state.postLock?.isLocked ?? false; | |||
There was a problem hiding this comment.
ℹ️ This is irrelevant change to the issue. Without this certain unit tests started to fail complaining about postLock being undefined in the state. This will just make sure that it has valid fallback.
There was a problem hiding this comment.
I think in the test file we usually just made sure a default is set. So for consistency probably best to update those tests too.
| // In either case, we don't want to accidentally loose the changes made by other user. | ||
| // If we perform server side autosave, and the post is in draft, previous changes will be overwritten in the database. | ||
| // Hence, we will only perform local autosave. | ||
| autosave( { local: true } ); |
There was a problem hiding this comment.
ℹ️ Ideally, isPostLocked check (which is part of isEditedPostSaveable check which in turn is part of isEditedPostAutosaveable) should be able to prevent this server autosave request. But somehow it seems like the data store at point of time in code execution is not properly updated. Hence, I'm still observing the network request for the autosave in the browser. Hence, I decided to put local: true to prevent the server autosave.
There was a problem hiding this comment.
If we perform server side autosave, and the post is in draft, previous changes will be overwritten in the database.
I don't think that's true. It creates a new autosave, not overriding any of the other user's changes. The original post and the other user's autosave both still exist.
So this change would break that.
FWIW it's possible to write e2e tests around post locking and autosaves... Sounds like now would be a good time to do that :-)
| // update login state to true. | ||
| updateCurrentUserSession( { isActive: true } ); | ||
| } | ||
| } |
There was a problem hiding this comment.
ℹ️ This whole function (which is managing the current user session in data store) can be questionable and there could be better way to handle this. So I'm happy to take any advise of improvement here.
| isLocked, | ||
| isTakeover, | ||
| user, | ||
| postId, | ||
| postLockUtils, | ||
| activePostLock, | ||
| postType, | ||
| isCurrentUserSessionActive, |
There was a problem hiding this comment.
ℹ️ For now, this data (isCurrentUserSessionActive) is only being used in this component and could well be a local state instead of being in the data store. But I can think of few use cases where we would want to check for current user sessions. I myself thought about using the same in AutosaveMonitor component. (This was when I was exploring potential fix in this PR. Eventually the need did not arise. So I removed that code before committing.) But the point is, there is a use case and can be a need in future.
|
Thanks for tackling this. I was able to reproduce the error in trunk, but was not able to get your branch working in my wp-env environment. Could you rebase the branch and I'll try again? This does look like a good approach. I initially was thinking that this should be rejecting the updates on the server side, but it looks like that is already happening and the client is working around that rejection. |
|
@georgeh Sorry, I've not been able to get back to this PR. I got busy with other things. I'll try to revive this PR this weekend. |
desaiuditd
force-pushed
the
fix/prevent-autosave-when-post-is-locked
branch
4 times, most recently
from
5d2f23b
to
4179f51
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @georgeh, @jayhill90. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
desaiuditd
force-pushed
the
fix/prevent-autosave-when-post-is-locked
branch
from
4179f51
to
6576ebc
Compare
So that we can prevent saving if the post is locked. We also prevent disabling Preview button when the post is not saveable. This is ripple effect of adding isPostLocked check in `isEditedPostSaveable` selector. We can safely remove this check on `Preview` button, because #32341 will prevent any save actions under the hood.
- Prevent autosave right before releasing the post lock. - Update current user session as per the heartbeat response. - We need to prevent save/update actions if the post is locked right after user logs back in on the same page. To do this, we need to show the modal as soon as user logs back in. Hence, we will re-trigger heartbeat right after nonce is renewed, so that we can update isPostLocked in data store.
desaiuditd
force-pushed
the
fix/prevent-autosave-when-post-is-locked
branch
from
6576ebc
to
1ccdeda
Compare
|
@georgeh I think this is ready for review. |
|
@desaiuditd sorry I haven't been keeping up with the project and am not in a position to re-review |
|
@desrosj @Mamaduka @mrfoxtalbot @johnbillion @aristath @ellatrix @ntsekouras @nerrad @jsnajdr @tyxla @andrewserong How can I move this PR forward? Can any of you help to review and test it? |
|
I've tested and verified on my local after syncing my branch with latest |
|
@WordPress/gutenberg-core @WordPress/gutenberg Apologies for a wider ping here. But this PR has got stale without any attention for quite some time now. Can I get some feedback on the code changes and help to review, test and move it forward? Don't want it to become stale again. |
|
Hi @desaiuditd 👋 I'm looking at this, trying to understand how do the heartbeats and post locks work. One thing that immediately comes to mind are the |
I once built a custom REST endpoint for post locking so that I don't have to use the heartbeat API: |
|
@spacedmonkey proposed porting that or a similar API - #37789. |
| @@ -106,8 +106,8 @@ export default function PostPreviewButton( { | |||
| role, | |||
| onPreview, | |||
| } ) { | |||
| const { postId, currentPostLink, previewLink, isSaveable, isViewable } = | |||
| useSelect( ( select ) => { | |||
| const { postId, currentPostLink, previewLink, isViewable } = useSelect( | |||
There was a problem hiding this comment.
Why remove the isSaveable check in this component?
isEditedPostSaveable returns false if the post is considered empty and it doesn't make sense to preview an empty post.
| @@ -139,22 +139,6 @@ describe( 'PostPreviewButton', () => { | |||
| ).toBeInTheDocument(); | |||
| } ); | |||
|
|
|||
| it( 'should be disabled if post is not saveable.', () => { | |||
There was a problem hiding this comment.
As per my other comment, it doesn't seem right to remove this functionality.
| @@ -1128,7 +1132,7 @@ export function getPermalinkParts( state ) { | |||
| * @return {boolean} Is locked. | |||
| */ | |||
| export function isPostLocked( state ) { | |||
| return state.postLock.isLocked; | |||
| return state.postLock?.isLocked ?? false; | |||
There was a problem hiding this comment.
I think in the test file we usually just made sure a default is set. So for consistency probably best to update those tests too.
| // In either case, we don't want to accidentally loose the changes made by other user. | ||
| // If we perform server side autosave, and the post is in draft, previous changes will be overwritten in the database. | ||
| // Hence, we will only perform local autosave. | ||
| autosave( { local: true } ); |
There was a problem hiding this comment.
If we perform server side autosave, and the post is in draft, previous changes will be overwritten in the database.
I don't think that's true. It creates a new autosave, not overriding any of the other user's changes. The original post and the other user's autosave both still exist.
So this change would break that.
FWIW it's possible to write e2e tests around post locking and autosaves... Sounds like now would be a good time to do that :-)
| @@ -141,7 +211,7 @@ export default function PostLockedModal() { | |||
| removeAction( 'heartbeat.tick', hookName ); | |||
| window.removeEventListener( 'beforeunload', releasePostLock ); | |||
| }; | |||
| }, [] ); | |||
| }, [ isDraft, isLocked ] ); | |||
There was a problem hiding this comment.
Is this intentional? More dependencies means the effect is triggered more often. I think an empty dependencies array was deliberately chosen here.
| @@ -185,6 +186,21 @@ export function getCurrentUser( state: State ): ET.User< 'edit' > { | |||
| return state.currentUser; | |||
| } | |||
|
|
|||
| /** | |||
| *Returns a boolean flag for current user's active login session. | |||
There was a problem hiding this comment.
| *Returns a boolean flag for current user's active login session. | |
| * Returns a boolean flag for current user's active login session. |
| // Bail early, if the heartbeat has neither auth check data attribute nor nonce expiry attribute. | ||
| if ( | ||
| ! [ 'wp-auth-check', 'nonces_expired' ].some( ( attr ) => | ||
| Object.prototype.hasOwnProperty.call( data, attr ) | ||
| ) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
auth-check.js in core only looks at wp-auth-check, not nonces_expired, see https://github.com/WordPress/wordpress-develop/blob/7b24083859c37d8ac75b9549f458bc389610eaef/src/js/_enqueues/lib/auth-check.js#L162-L168
So I'd remove the nonces_expired check here.
And then simplify to:
| // Bail early, if the heartbeat has neither auth check data attribute nor nonce expiry attribute. | |
| if ( | |
| ! [ 'wp-auth-check', 'nonces_expired' ].some( ( attr ) => | |
| Object.prototype.hasOwnProperty.call( data, attr ) | |
| ) | |
| ) { | |
| return; | |
| } | |
| if ( ! 'wp-auth-check' in data ) { | |
| return; | |
| } |
There was a problem hiding this comment.
If we used apiFetch for the heartbeat request, then we wouldn't need to care about nonces at all, because apiFetch handles them internally (nonce middleware). Automatically requesting a new one when the old one expires etc.
I'm not sure if admin-ajax.php is compatible with apiFetch or whether we'd need to create a REST endpoint first.
There was a problem hiding this comment.
No that would require a REST endpoint. See the ither comments about that.
This comment here is still relevant for admin-ajax
| * | ||
| * @return {Object} Action object. | ||
| */ | ||
| export function __experimentalUpdateCurrentUserSession( currentUserSession ) { |
There was a problem hiding this comment.
This looks like local state. core-data was not really mean to store things like that. So can you explain more the reasoning here and what this is about?
There was a problem hiding this comment.
I think this state could be very easily moved to the editor store, to the state.postLock object. That object already has plenty of fields (isLocked, isTakeover, user, ...), so why not add another isUserActive field there?
All other aspects of this user session check should be integrated into the post lock state, too. Why add another hook to heartbeat.tick, if we can just expand the existing receivePostLock function?
There was a problem hiding this comment.
Moving this to the postLock object sounds confusing. Post lock state is coming from an entirely different API response and it's easy to accidentally override postLock.isUserActive (or vice-versa) in the process.
There was a problem hiding this comment.
Then it could be renamed to state.heartbeat, with subfields for post lock, auth check and possibly other things. They are all requested together, and also used together.
| } | ||
| } | ||
|
|
||
| addAction( 'heartbeat.tick', checkUserLoginHookName, checkUserLogin ); |
There was a problem hiding this comment.
We already subscribe to this action in the hook down here. Was it a deliberate choice to separate these two logics. The reasoning is not clear to me.
Description
Fixes #13509. Potentially addresses #21236, #42400, #36118 as well. Also fixes a bug described below.
Steps to reproduce
How has this been tested?
With the fix in this PR, I tested above reproduction steps. After User A re-logs in, there's not autosave request triggered, User A immediately sees the
PostLockedModalpreventing any further updates and User B is not loosing the content anymore.Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).