search

10up / distributor

Share content between your websites.
https://distributorplugin.com
GNU General Public License v2.0
628 stars 154 forks source link

Limited external connection warning incorrect #1140

Closed theskinnyghost closed 11 months ago

theskinnyghost commented 11 months ago

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:

theskinnyghost commented 11 months ago

@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 image

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

theskinnyghost commented 11 months ago

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 commented 11 months ago

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.

https://github.com/10up/distributor/blob/0387c94509f4dbde5781628671b0b58abf5e3884/tests/cypress/config.js#L29-L48

I can take a look at the changes required.

theskinnyghost commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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