Add end 2 end test: should not revert title during a preview right after a save draft #7953

jorgefilipecosta · 2018-07-13T14:23:57Z

Description

Adds an end 2 end test that verifies the we don't regress on issue #7927.

The test does the following:
Adds a post.
Writes aaaa in title and clicks save draft.
Presses preview.
See the title is correctly previewed as aaaa.
Goes back to the editor ands appends bbbb to the title, presses save draft. Presses preview right after the post is saved.
Verify that on the preview of the post we see 'aaaabbbb' as the title of the post.

How has this been tested?

Verify end 2 end tests run with success.

aduth

This seems like something going wrong with how we're trying to fall back to post values in creating the autosave payload:

gutenberg/packages/editor/src/store/effects/posts.js

Lines 107 to 114 in 25f364b

    
           // Ensure autosaves contain all expected fields, using autosave or 
        
           // post values as fallback if not otherwise included in edits. 
        
           toSend = { 
        
           	...pick( post, [ 'title', 'content', 'excerpt' ] ), 
        
           	...getAutosave( state ), 
        
           	...toSend, 
        
           	parent: post.id, 
        
           };

Why is it that without the proposed change, title is not set the edited value?

It's also not entirely sensible that we're even calling the autosave endpoint in the first place, since we've just saved the post and there's nothing to be autosaved.

Finally, we should consider excerpt as well, since an autosave is of its title, content, and excerpt. Following the original instructions, but using excerpt instead of the title, the misbehavior still exists.

You might need to update your theme to make this apparent. I included <?php the_excerpt(); ?> in my theme's single.php for example.

Is there any way we can include a test for this?

aduth · 2018-09-13T19:40:15Z

Are you able to resume work here?

jorgefilipecosta · 2018-09-18T16:49:27Z

Are you able to resume work here?

Sure @aduth I'm sorry for the delay I totally missed your comment in the middle of other notifications.

Edited: I think I already understand the problem, I'm working in updating this PR.

jorgefilipecosta · 2018-09-18T19:14:31Z

Hi @aduth thank you for the insights, I updated this PR to use another fix. Now on the preview, we have a new condition that makes sure the autosave is not triggered if the post is not dirty. That may have been the original intention as the isDirty flag was being retrieved from the state but not used anywhere on the component.
End 2 End tests are failing I'm debugging to see if the problem is real or tests need to be updated. I will also look into adding an end 2 end test against the problem we are fixing.

jorgefilipecosta · 2018-09-20T11:31:07Z

End 2 End tests are executing with success, I think it was some problem in Travis that made them fail. I executed them and they are passing. I added a new test case for this bug (that fails on master).

aduth · 2018-10-05T21:43:39Z

packages/editor/src/components/post-preview-button/index.js

 			isNew: isEditedPostNew(),
 			isSaveable: isEditedPostSaveable(),
-			isAutosaveable: isEditedPostAutosaveable(),
+			isAutosaveable: isEditedPostAutosaveable() && isEditedPostDirty(),


Should isEditedPostAutosaveable encompass isEditedPostDirty?

talldan · 2018-10-10T02:41:47Z

packages/editor/src/store/selectors.js

-	if ( ! isEditedPostSaveable( state ) ) {
+	// A post needs to be in dirty state and,
+	// a post must contain a title, an excerpt, or non-empty content to be valid for autosaving.
+	if ( ! isEditedPostDirty( state ) || ! isEditedPostSaveable( state ) ) {


I noticed this comment previously about why isEditedPostDirty can't be used in isEditedPostSaveable and wondered whether it applies here as well:

gutenberg/packages/editor/src/store/selectors.js

Lines 346 to 349 in 1349d23

// TODO: Post should not be saveable if not dirty. Cannot be added here at

// this time since posts where meta boxes are present can be saved even if

// the post is not dirty. Currently this restriction is imposed at UI, but

// should be moved here.

I don't think so, as my expectation is that meta boxes don't trigger autosaves. I think what happens is that isDirty does not account for meta boxes, so if meta boxes are present the save button is always available.

talldan · 2018-10-10T03:02:05Z

I'm a bit unclear on why previous autosave data is even included in the payload for a new autosave:

gutenberg/packages/editor/src/store/effects/posts.js

Lines 110 to 115 in 5c19314

    
           toSend = { 
        
           	...pick( post, [ 'title', 'content', 'excerpt' ] ), 
        
           	...getAutosave( state ), 
        
           	...toSend, 
        
           	parent: post.id, 
        
           };

Wouldn't just post and toSend make up the changes from the post the user is viewing?

jorgefilipecosta · 2018-10-12T09:44:29Z

Hi @talldan,

Wouldn't just post and toSend make up the changes from the post the user is viewing?

I did not have knowledge about how autosaving works but I did some digging, and it looks like autosave payloads always need to include the three fields as specified in the comments 'title', 'content', 'excerpt'. When for example title is updated, and an autosave happens, if after we change content, toSend does not contain the title, and the post includes the title before the autosave so for us to send the most recent title in a second autosave we need retrieve the title from the previous autosave.

jorgefilipecosta · 2018-11-13T23:12:16Z

This PR passed by a rebase, and some code changes were applied. In summary, this PR just adds a rule, "A post non dirty post is not autosaveable", the other changes are test cases and removing of isDirty check when it is already included in the autosave check.

I think the required improvements were applied.

aduth · 2018-11-27T18:40:24Z

@jorgefilipecosta In trying to reproduce the original issue in master, I'm finding it to be working as intended. Can you verify whether the bug is still present? I'm thinking it may have been incidentally resolved by @youknowriad 's #12097 .

Squashed commits: [531e34b] Add end 2 end test [22a1c81] Autosave: Fix: Autosave may destroy save incorrect post title

jorgefilipecosta · 2018-11-27T19:23:29Z

Hi @aduth that you for retesting this. You are right the issue was solved, I confirmed in manual tests and the end 2 end test also confirms it.
I updated this PR to just contain the end 2 end test I think it is still valuable to get it in to avoid a future regression.

aduth

Looks good 👍

jorgefilipecosta changed the title ~~Autosave: Fix: Autosave may destroy save incorrect post title~~ Autosave: Fix: Autosave may save incorrect post title Jul 13, 2018

jorgefilipecosta requested a review from aduth July 16, 2018 19:13

jorgefilipecosta force-pushed the fix/autosave-may-incorrectly-save-title branch from 0202a14 to c35fa8c Compare August 1, 2018 15:04

jorgefilipecosta requested a review from a team August 1, 2018 15:07

aduth previously requested changes Aug 1, 2018

View reviewed changes

aduth added the [Feature] Saving Related to saving functionality label Aug 1, 2018

jorgefilipecosta force-pushed the fix/autosave-may-incorrectly-save-title branch 2 times, most recently from 6fc2105 to 5c19314 Compare September 18, 2018 18:57

jorgefilipecosta force-pushed the fix/autosave-may-incorrectly-save-title branch from 5c19314 to e3598ce Compare September 20, 2018 00:52

jorgefilipecosta requested a review from a team October 5, 2018 20:53

jorgefilipecosta force-pushed the fix/autosave-may-incorrectly-save-title branch from e3598ce to 421bf10 Compare October 5, 2018 20:55

aduth reviewed Oct 5, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/autosave-may-incorrectly-save-title branch 2 times, most recently from 7b86a0f to 0fd7fb7 Compare October 8, 2018 22:51

talldan reviewed Oct 10, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/autosave-may-incorrectly-save-title branch 5 times, most recently from 4f7e103 to 15178ae Compare November 12, 2018 16:46

earnjam mentioned this pull request Nov 19, 2018

Perform a full save when previewing drafts #12047

Closed

Refactor isEditedPostAutosaveable selector. (+2 squashed commits)

31c1a11

Squashed commits: [531e34b] Add end 2 end test [22a1c81] Autosave: Fix: Autosave may destroy save incorrect post title

jorgefilipecosta force-pushed the fix/autosave-may-incorrectly-save-title branch from 15178ae to 31c1a11 Compare November 27, 2018 19:14

jorgefilipecosta changed the title ~~Autosave: Fix: Autosave may save incorrect post title~~ Add end 2 end test: should not revert title during a preview right after a save draft Nov 27, 2018

aduth approved these changes Nov 27, 2018

View reviewed changes

jorgefilipecosta merged commit f97617f into master Nov 27, 2018

jorgefilipecosta deleted the fix/autosave-may-incorrectly-save-title branch November 27, 2018 21:33

jorgefilipecosta added this to the 4.6 milestone Nov 28, 2018

youknowriad modified the milestones: 4.6, 4.7 Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add end 2 end test: should not revert title during a preview right after a save draft #7953

Add end 2 end test: should not revert title during a preview right after a save draft #7953

jorgefilipecosta commented Jul 13, 2018 •

edited

Loading

aduth left a comment

aduth commented Sep 13, 2018

jorgefilipecosta commented Sep 18, 2018 •

edited

Loading

jorgefilipecosta commented Sep 18, 2018 •

edited

Loading

jorgefilipecosta commented Sep 20, 2018 •

edited

Loading

aduth Oct 5, 2018

talldan Oct 10, 2018 •

edited

Loading

jorgefilipecosta Oct 10, 2018

talldan commented Oct 10, 2018 •

edited

Loading

jorgefilipecosta commented Oct 12, 2018

jorgefilipecosta commented Nov 13, 2018

aduth commented Nov 27, 2018

jorgefilipecosta commented Nov 27, 2018

aduth left a comment

	// Ensure autosaves contain all expected fields, using autosave or
	// post values as fallback if not otherwise included in edits.
	toSend = {
	...pick( post, [ 'title', 'content', 'excerpt' ] ),
	...getAutosave( state ),
	...toSend,
	parent: post.id,
	};

	// TODO: Post should not be saveable if not dirty. Cannot be added here at
	// this time since posts where meta boxes are present can be saved even if
	// the post is not dirty. Currently this restriction is imposed at UI, but
	// should be moved here.

Add end 2 end test: should not revert title during a preview right after a save draft #7953

Add end 2 end test: should not revert title during a preview right after a save draft #7953

Conversation

jorgefilipecosta commented Jul 13, 2018 • edited Loading

Description

How has this been tested?

aduth left a comment

Choose a reason for hiding this comment

aduth commented Sep 13, 2018

jorgefilipecosta commented Sep 18, 2018 • edited Loading

jorgefilipecosta commented Sep 18, 2018 • edited Loading

jorgefilipecosta commented Sep 20, 2018 • edited Loading

aduth Oct 5, 2018

Choose a reason for hiding this comment

talldan Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta Oct 10, 2018

Choose a reason for hiding this comment

talldan commented Oct 10, 2018 • edited Loading

jorgefilipecosta commented Oct 12, 2018

jorgefilipecosta commented Nov 13, 2018

aduth commented Nov 27, 2018

jorgefilipecosta commented Nov 27, 2018

aduth left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jul 13, 2018 •

edited

Loading

jorgefilipecosta commented Sep 18, 2018 •

edited

Loading

jorgefilipecosta commented Sep 18, 2018 •

edited

Loading

jorgefilipecosta commented Sep 20, 2018 •

edited

Loading

talldan Oct 10, 2018 •

edited

Loading

talldan commented Oct 10, 2018 •

edited

Loading