Multi-entity saving: Cannot publish a post with site entity deselected

[ramonjd]

Description

Before publishing a new post that contains post and site entities, deselecting the site entity, then hitting Publish triggers a Javascript error. The result is that I cannot publish a post.

The error is this.props.setEntitiesSavedStatesCallback is not a function
It looks like the prop setEntitiesSavedStatesCallback is not passed to the post publish button from the post publish panel.

First noticed while testing Site Logo: Add option to set site icon from Site Logo block
#35892 in #35892 (comment)

Step-by-step reproduction instructions

1. Create a new post
2. Insert a Site Logo block. Add or upload an image.
3. Insert some text or a heading.
4. Hit Publish to publish the post.
5. In the pre-publish save menu, deselect Logo, then hit Save, then Publish.

What I expected

That I could publish the post, or that I'd be given feedback as to why I couldn't publish the post without saving the site logo.

What happened

Nothing in the editor, but the console throws the following error:

Screenshots, screen recording, code snippet

Nov-01-2021.10-11-55.mp4

Environment info

• WordPress 5.8.1
• Gutenberg trunk
• TwentyTwentyOne and TT1 Blocks

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[carolinan]

I have not been able to reproduce this (current trunk at c5e695d)

[stacimc]

I pulled current trunk and was able to reproduce this; to re-emphasize, it only happens when publishing a brand new post for the first time.

I was also able to reproduce this with other non-post entities than Site Logo. For example I:

1. Start a new post view wp-admin/post-new.php
2. Insert the Template Part block and open the existing Footer template
3. Make some changes to the template
4. Click 'Publish' to publish the post
5. Deselect the Footer template from the pre-publish menu
6. Hit Save, then Publish

I get the same error here as well.

[ramonjd]

Was looking at this again. I believe there's some race condition in the store, or we're not correctly removing entity edits when we save.

When the entity checkboxes are saved, and we update via saveSpecifiedEntityEdits(), by the time we hit publish the hasNonPostEntityChanges() selector still returns true because __experimentalGetDirtyEntityRecords still thinks there are dirty entity records records [{key: undefined, title: 'a8c-wp-env', name: 'site', kind: 'root'}].

I think.

[ramonjd]

Passing down the setEntitiesSavedStatesCallback prop to <PostPublishButton /> creates the following effect:

Nov-10-2021.20-51-59.mp4

I can't save the post without also saving the site entity.

[ockham]

Was looking at this again. I believe there's some race condition in the store, or we're not correctly removing entity edits when we save.

When the entity checkboxes are saved, and we update via saveSpecifiedEntityEdits(), by the time we hit publish the hasNonPostEntityChanges() selector still returns true because __experimentalGetDirtyEntityRecords still thinks there are dirty entity records records [{key: undefined, title: 'a8c-wp-env', name: 'site', kind: 'root'}].

I think.

PR to fix this behavior here: #36573 (but that doesn't fix the issue entirely).

[ockham]

Here's an e2e test for this issue that I wrote (based on an existing one). Note that it doesn't actually fail (it just prints the TypeError to the console from which it's run, but passes). I hope it'll still come in handy in attempting to fix this issue, and later prevent it from re-appearing.

(I think that the runtime JS error is eaten by @wordpress/jest-console. I thus added expect( console ).not.toHaveErrored(), but that's not causing the test to fail either. @gziolo Is that expected? Is there any other way to make an e2e test fail if there's a JS console error in the browser?)

diff --git a/packages/e2e-tests/specs/experiments/multi-entity-saving.test.js b/packages/e2e-tests/specs/experiments/multi-entity-saving.test.js
index a8429cc24a..db9e5444e1 100644
--- a/packages/e2e-tests/specs/experiments/multi-entity-saving.test.js
+++ b/packages/e2e-tests/specs/experiments/multi-entity-saving.test.js
@@ -229,6 +229,47 @@ describe( 'Multi-entity save flow', () => {
 					.saveEditedEntityRecord( 'root', 'site', undefined );
 			} );
 		} );
+
+		// Regression: https://github.com/WordPress/gutenberg/issues/36096
+		it( "Can publish even if some changes haven't been saved", async () => {
+			await createNewPost();
+			//await disablePrePublishChecks();
+
+			await insertBlock( 'Site Title' );
+			// Ensure title is retrieved before typing.
+			await page.waitForXPath( '//a[contains(text(), "gutenberg")]' );
+			const editableSiteTitleSelector =
+				'.wp-block-site-title a[contenteditable="true"]';
+			await page.waitForSelector( editableSiteTitleSelector );
+			await page.focus( editableSiteTitleSelector );
+			await page.keyboard.type( '...' );
+
+			await clickButton( 'Publish' );
+			await page.waitForSelector( savePanelSelector );
+			const checkboxInputs = await page.$$( checkboxInputSelector );
+			expect( checkboxInputs ).toHaveLength( 2 );
+
+			await checkboxInputs[ 0 ].click();
+			await page.click( entitiesSaveSelector );
+
+			await clickButton( 'Save' );
+			await page.waitForSelector( publishPanelSelector );
+
+			await clickButton( 'Publish' ); // Make sure we can publish.
+			expect( console ).not.toHaveErrored();
+
+			// Reset site entity to default value to not affect other tests.
+			await page.evaluate( () => {
+				wp.data
+					.dispatch( 'core' )
+					.editEntityRecord( 'root', 'site', undefined, {
+						title: 'gutenberg',
+					} );
+				wp.data
+					.dispatch( 'core' )
+					.saveEditedEntityRecord( 'root', 'site', undefined );
+			} );
+		} );
 	} );
 
 	describe( 'Site Editor', () => {

[gziolo]

@ockham, there is a code that should catch the errors and warnings printed on the Browser Console:

gutenberg/packages/e2e-tests/config/setup-test-framework.js

Line 106 in 4384d9c

function observeConsoleLogging() {

The check you included in the example operates only on the test suite scope. It would catch issues triggered in the code executed by Jest. That’s why we listen for errors on the page with a listener using the code I shared above.

[ockham]

@ockham, there is a code that should catch the errors and warnings printed on the Browser Console:

gutenberg/packages/e2e-tests/config/setup-test-framework.js

Line 106 in 4384d9c

function observeConsoleLogging() {

The check you included in the example operates only on the test suite scope. It would catch issues triggered in the code executed by Jest. That’s why we listen for errors on the page with a listener using the code I shared above.

Thanks for replying @gziolo! I'm not sure I understand 100% -- can you clarify what needs to be done in order to have that console TypeError cause the test to fail? 😅

[gziolo]

can you clarify what needs to be done in order to have that console TypeError cause the test to fail? 😅

It looks like you want to detect exceptions thrown. Do we have any onError handler configured? There are also Error Boundaries from React that might intercept errors thrown.

[paaljoachim]

It would be helpful to get an update on how this PR is coming along.
Thank you!

[ockham]

It would be helpful to get an update on how this PR is coming along. Thank you!

Right, apologies for the lack of updates. The relevant code is unfortunately rather complex (nested callbacks etc) -- I think it's a result of the Publish/Update/Save logic requiring more and more of implicit state as multi-entity saving etc were implemented, and trying to fit that into the existing framework. Arguably, it could use a refactor with a somewheat different architecture (more explicit state?), based on the current requirements.

That said, I also had to drop the ball on this one since I was helping out with other WP 5.9 To-do items that were higher priority. I'm afraid I don't have a clear ETA when I'll be able to get back to this issue 😕

[paaljoachim]

Hey Bernie

The PR and various refactors will come along in its own pace...:)
Thanks for the update!

[stacimc]

I took a look at this and I think I was able to get it working. I understand if a refactor makes sense moving forward, but #37383 has a possible fix for the meantime and some notes for what I think is happening here, so hopefully it's helpful!

[ockham]

Filed draft PR with e2e test based on #36096 (comment): #37408