Limited external connection warning incorrect

[theskinnyghost]

Description of the Change

Closes #1071

How to test the Change

1. Go to the WP Dashboard > Distributor > External Connections > Add new
2. Click Manually Set Up Connection.
3. Enter an incorrect username and password combination
4. Enter a valid REST API endpoint in the URL field for a site that is running Distributor
5. Instead of the yellow warning, you should see a red error with a Authentication failed due to insufficient or invalid credentials. message instead
6. The push and pull permissions list will show no post types can be pushed or pulled
7. Save the connection
8. Visit Distributor > Pull Content
9. The new connection will not be listed as the site does not have any pullable content types
10. Publish a new post
11. Click "Distribute"
12. The new external connection will not be listed as the site does not have any pushable content types

Changelog Entry

Changed external connection status error messages when checking status

Credits

Props @theskinnyghost

Checklist:

• I agree to follow this project's Code of Conduct.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests pass.

[theskinnyghost]

@peterwilsoncc I tried to run the cypress tests locally but wasn't able to so I'm not sure if I need to make any additional setup configuration to run them properly. Here's the error that I get when I run npm run cypress:run

Also, is this the test I would need to update?
https://github.com/10up/distributor/blob/36d1c76/tests/cypress/e2e/external-connection.test.js#L99

[peterwilsoncc]

Thanks for the PR, Luca.

This is my fault but I the rest-api endpoint needs an update in the function check_post_types_permissions(), the permission check loop should be:

foreach ( $types as $type ) {
	$caps = $type->cap;

	if ( current_user_can( $caps->edit_posts ) ) {
		$response['can_post'][] = $type->name;
	}

	if ( current_user_can( $caps->edit_posts ) && current_user_can( $caps->create_posts ) && current_user_can( $caps->publish_posts ) ) {
		$response['can_post'][] = $type->name;
	}
}

While testing, I noticed that if Distributor is not installed it shows the old limited connection message, as in this screenshot. This too will need to be updated to an error.

You're correct, the test you've linked to in external-connection.test.js is the one that needs an update.

Getting Cypress running locally can be a little fiddly, were you able to get the environment running in Docker or did it fail before then?

[theskinnyghost]

Hey @peterwilsoncc, thanks for your feedback.

I made an update to show an error for the use case you mentioned above (when distributor is not installed).

I also updated the check_post_types_permissions() logic as per your suggestion but I'm not sure I did it right so I left a comment for you to check.

RE: Cypress Tests – I only ran npm run cypress:run and that's the error I got. Do I need to run additional commands? I use Local Flywheel not Docker.

[peterwilsoncc]

RE: Cypress Tests – I only ran npm run cypress:run and that's the error I got. Do I need to run additional commands? I use Local Flywheel not Docker.

The E2E tests use @wordpress/env which requires Docker. You may be able to change the URL returned by this function to use local but I've never had much luck doing that as a vagrant user.

distributor/tests/cypress/config.js

Lines 29 to 48 in 0387c94

	/**
	* Set WP URL as baseUrl in Cypress config.
	*
	* @param {Function} on function that used to register listeners on various events.
	* @param {Object} config Cypress Config object.
	* @return {Object} Updated Cypress Config object.
	*/
	const setBaseUrl = async ( on, config ) => {
	const wpEnvConfig = await readConfig( 'wp-env' );
	if ( wpEnvConfig ) {
	const port = wpEnvConfig.env.tests.port || null;
	if ( port ) {
	config.baseUrl = 'http://localhost:80/';
	}
	}
	return config;
	};

I can take a look at the changes required.

[peterwilsoncc]

I retested and was still seeing some unexpected warnings rather than errors, so have added a few more suggestions.

With these changes, I think we'll then be able to remove the else if beginning on line 367 -- it's outside the diff so I'm unable to include it as an inline suggestion.

[theskinnyghost]

@peterwilsoncc thanks again for your feedback. I implemented the changes you suggested but wasn't totally sure we could remove the else if from line 367 as that would completely remove warnings. Is that what you'd like to achieve here?

re: tests - I was able to run the tests this time by running npm env:start before runningnpm run cypress:run. However, a lot of tests are failing (even when I use the develop or trunk branch). I wonder if I'm missing anything here?

[peterwilsoncc]

wasn't totally sure we could remove the else if from line 367 as that would completely remove warnings. Is that what you'd like to achieve here?

Yes, that's what I was after. The two conditions response.data.errors.no_distributor || ! response.data.can_post.length are both included in the first block now and throw errors instead of warnings.

A downside to switching to the dedicated Distributor endpoint is that it's basically all-or-nothing for connections at the moment. They'll either error or succeed, we're no longer able to have the partial connections that used to trigger warnings.

As the block-editor becomes more important, the partial connections of old become less effective as the block editor requires access to the raw content.

re: tests - I was able to run the tests this time by running npm env:start before runningnpm run cypress:run. However, a lot of tests are failing (even when I use the develop or trunk branch). I wonder if I'm missing anything here?

I'll test from scratch locally and reach out via the 10up Slack. It's possible the docs need to be updated.

[theskinnyghost]

Thanks @peterwilsoncc, I've updated the code and removed the warnings since they aren't applicable anymore. I also updated the tests and removed the test for warnings.
I also think we should add an additional test to make sure the Distributor not installed on remote site. error appears when that's the case but I'm not sure how to set this up with the current setup and also wanted to double check with you if that's needed.
I was able to run the tests just fine following the instructions you provided on 10up's Slack, thanks! 😄

[peterwilsoncc]

For others, these are the steps currently required to get the commands required to get e2e tests running locally.

Please ensure Docker is running before running these commands.

npm run env:destroy // Just to get back to scratch
npm run env:start
npm run copy-htaccess
npm run cypress:run

[peterwilsoncc]

Thanks Luca, this looks good to me.

Thanks for your patience during the review process, quite a bit had changed.