Social: Fix connection_test_result call merging issue #38367

gmjuhasz · 2024-07-17T12:26:45Z

Fixes https://github.com/Automattic/jetpack-reach/issues/448

On slower connections there is a scenario which could happen that messes up the connection state:

On page load the test_connections call fires, but takes 10-15 seconds to complete
In the meantime the user adds/removes a connection, changing the state
If the addition/remove of connection finishes faster, the test_connections call will mess up the state when it finishes, it will either remove the new connection from the UI, or add back the removed one.

If you have a speedy network it's harder to reproduce, so one solution for that is to make changes in the mergeConnections function in the projects/js-packages/publicize-components/src/social-store/actions/connection-data.js file, make the inner function async, and add the await line at the start like so:

export function mergeConnections( freshConnections ) {
-	return function ( { dispatch, select } ) {
+	return async function ( { dispatch, select } ) {
+		await new Promise( resolve => setTimeout( resolve, 10000 ) );

Issue presented with connection removal - the connection gets added back after `test_connections` finishes:

CleanShot.2024-07-17.at.14.20.50.png.mp4

Issue presented with the connection addition - the connection gets removed after `test_connections` finishes:

CleanShot.2024-07-17.at.14.23.01.png.mp4

Proposed changes:

Handles scenarios where prevConnections and freshConnections is not the same
- if a connection was added, we just add it to freshConnections from prevConnections
- if a connection was removed, we remove it from freshConnections so it won't get added back

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

If your network is too fast follow the instructions at the top of the PR to include the timeout
Go to the post editor, switch to Jetpack tab and quickly add a new connection. It should not disappear after the test_connections call finishes
Refresh the page, and now quickly remove the connection. It should not get added back after the test_connections call finishes.

github-actions · 2024-07-17T12:27:40Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the fix/social-connection-test-result-logic branch.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack fix/social-connection-test-result-logic

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2024-07-17T12:27:44Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

gmjuhasz · 2024-07-17T12:42:41Z

...ts/js-packages/publicize-components/src/hooks/use-sync-post-data-to-store/test/index.test.js

@@ -13,7 +13,7 @@ import {
 const connections = connectionsList.map( connection => ( { ...connection, enabled: true } ) );

 const post = {
-	jetpack_publicize_connections: [ connections[ 0 ] ],
+	jetpack_publicize_connections: [ connections ],


cc @manzoorwanijk because you wrote this testcase:
I think this change won't alter the testing power of this test, I also think that it should never be the case that prevConnections is not the same length as freshConnections

Shouldn't it then be this instead?

Suggested change

jetpack_publicize_connections: [ connections ],

jetpack_publicize_connections: connections,

manzoorwanijk · 2024-07-18T06:32:53Z

projects/js-packages/publicize-components/src/social-store/actions/connection-data.js

+		// A connection was added after the test call started, so we need to add it to the list.
+		if ( prevConnections.length > freshConnections.length ) {
+			const addedConnection = prevConnections.find(
+				conn =>
+					! freshConnections.some( freshConn =>
+						freshConn.connection_id
+							? freshConn.connection_id === conn.connection_id
+							: freshConn.id === conn.id
+					)
+			);
+
+			if ( addedConnection ) {
+				connections.push( addedConnection );
+			}
+		}
+


This approach is problematic. Currently, if a post has connections saved in post meta (jetpack_publicize_connections), it's possible that the list is outdated and any of those connections would have been removed from connections management. Thus, connection test result resets the list and ensures that only the active connections are in the list.

With this change, it will add those removed connections back to the list which is not what we want.

Steps:

Add some connections

Create a post and publish or save it as draft.

Goto Social or Jetpack settings page and remove one or more connections

Go back to the post editor

On trunk, the removed connections should not be retained after test results, but with this change, they are retained.

manzoorwanijk

This needs a rethink. Please see the comments above.

gmjuhasz · 2024-07-18T10:25:29Z

Closing in favour on other solution in a clean PR - #38408

gmjuhasz added 2 commits July 17, 2024 14:08

Handle if connections were changed during the test_connections call

6a878f3

changelog

64fe33f

gmjuhasz requested a review from a team July 17, 2024 12:26

gmjuhasz self-assigned this Jul 17, 2024

gmjuhasz added the [Status] Needs Team Review label Jul 17, 2024

github-actions bot added [JS Package] Publicize Components RNA labels Jul 17, 2024

Fix test

d23be9b

github-actions bot added the [Tests] Includes Tests label Jul 17, 2024

gmjuhasz commented Jul 17, 2024

View reviewed changes

manzoorwanijk reviewed Jul 18, 2024

View reviewed changes

manzoorwanijk requested changes Jul 18, 2024

View reviewed changes

gmjuhasz closed this Jul 18, 2024

github-actions bot removed the [Status] Needs Team Review label Jul 18, 2024

manzoorwanijk mentioned this pull request Jul 18, 2024

Social: Fix parallel social connections requests mixing up UI state #38408

Merged

3 tasks

manzoorwanijk deleted the fix/social-connection-test-result-logic branch July 18, 2024 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Social: Fix connection_test_result call merging issue #38367

Social: Fix connection_test_result call merging issue #38367

gmjuhasz commented Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 17, 2024

gmjuhasz Jul 17, 2024

manzoorwanijk Jul 18, 2024

manzoorwanijk Jul 18, 2024 •

edited

Loading

manzoorwanijk left a comment

gmjuhasz commented Jul 18, 2024 •

edited by manzoorwanijk

Loading

	jetpack_publicize_connections: [ connections ],
	jetpack_publicize_connections: connections,

Social: Fix connection_test_result call merging issue #38367

Social: Fix connection_test_result call merging issue #38367

Conversation

gmjuhasz commented Jul 17, 2024 • edited Loading

Issue presented with connection removal - the connection gets added back after test_connections finishes:

Issue presented with the connection addition - the connection gets removed after test_connections finishes:

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Jul 17, 2024 • edited Loading

github-actions bot commented Jul 17, 2024

gmjuhasz Jul 17, 2024

Choose a reason for hiding this comment

manzoorwanijk Jul 18, 2024

Choose a reason for hiding this comment

manzoorwanijk Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

manzoorwanijk left a comment

Choose a reason for hiding this comment

gmjuhasz commented Jul 18, 2024 • edited by manzoorwanijk Loading

gmjuhasz commented Jul 17, 2024 •

edited

Loading

Issue presented with connection removal - the connection gets added back after `test_connections` finishes:

Issue presented with the connection addition - the connection gets removed after `test_connections` finishes:

github-actions bot commented Jul 17, 2024 •

edited

Loading

manzoorwanijk Jul 18, 2024 •

edited

Loading

gmjuhasz commented Jul 18, 2024 •

edited by manzoorwanijk

Loading