Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle publish tweet from staging/testing site. #161

Merged
merged 6 commits into from
May 19, 2022
Merged

Handle publish tweet from staging/testing site. #161

merged 6 commits into from
May 19, 2022

Conversation

iamdharmesh
Copy link
Member

Description of the Change

This PR adds support for detecting staging/testing environments and handling publish tweets from staging/testing/local environments to prevent publishing accidental tweets.

The plugin saves a live site URL to the options table with _[autoshare_siteurl]_ in the middle of it, to prevent it from getting replaced by a search-replace script during migraion. So, whenever the current wp site URL doesn't match with the saved live site URL, the plugin shows the notice to the admin user and stops publishing tweets. Admin users can enable publish tweets OR dismiss notice with keep publish tweet disabled.

image

Closes #39

Alternate Designs

Maybe we can move the credentials store from sb to wp-config.php, to prevent accidental tweets.

Possible Drawbacks

Didn't notice any yet.

Verification Process

  1. Create a website/blog with WP and setup AST on it
  2. Migrate the website to a different domain.
  3. You will see the notice related to the staging/testing site and publish tweets will be disabled.
  4. Test notice actions and make sure it works as expected.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Added - Handle publish tweets from staging/testing/local environments to prevent publishing accidental tweets.

Credits

Props @iamdharmesh @dinhtungdu

@iamdharmesh iamdharmesh self-assigned this May 13, 2022
@iamdharmesh iamdharmesh added this to the 1.2.0 milestone May 13, 2022
@iamdharmesh iamdharmesh marked this pull request as ready for review May 13, 2022 11:44
@iamdharmesh iamdharmesh requested review from a team and peterwilsoncc and removed request for a team May 13, 2022 11:45
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few notes inline.

To validate the URL, could you use a checksum instead of storing the value. Say sha1( site_url() )? As it's a checksum rather than a password, you don't need to worry about the recommendation the sha1() function been avoided for secure hashing.

Why have you used site_url() throughout instead of home_url()? The home URL is the address of the website, the site URL is the address of the WordPress install and the values can differ.

includes/class-ast-staging.php Outdated Show resolved Hide resolved
* @since 1.2.0
* @return bool Whether the site is a staging URL or not.
*/
public static function is_duplicate_site() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could do with number of comments, it's unclear what is been compared.

Can this use the WP_ENVIRONMENT_TYPE constant with the core function wp_get_environment_type()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function compares the site's current URL with the saved live URL in the DB to identify if the current env is prod or not. for WP_ENVIRONMENT_TYPE, I am not sure if all devs use this to env var to mark the environment. So, I am not feeling comfortable depending on this env var. what do you think?

* Filters value of "Is staging site?".
*
* @since 1.2.0
* @param boolean $is_duplicate Is staging site?.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param boolean $is_duplicate Is staging site?.
* @param boolean $is_duplicate Whether the site is a staging URL or not.

As there are staging, development and local sites, instead of $is_duplicate it might be clearer to use $is_production and reverse the logic of the checks.

@iamdharmesh
Copy link
Member Author

Hi @peterwilsoncc,

Thanks for the review and notice some important stuff to fix.

To validate the URL, could you use a checksum instead of storing the value. Say sha1( site_url() )? As it's a checksum rather than a password, you don't need to worry about the recommendation the sha1() function been avoided for secure hashing.

Yes, we can use sha1 here. but, I didn't use it to keep the URL in the value that allows for viewing and editing the URL directly in the database.

Why have you used site_url() throughout instead of home_url()? The home URL is the address of the website, the site URL is the address of the WordPress install and the values can differ.

No specific reason for that. I thought whatever we use, we have to compare it with the value of the same function (site_url() every time). So, I proceed with site_url(). please let me know if I missing anything here and it's better to use home_url() here.

Thanks.

@peterwilsoncc
Copy link
Contributor

Yes, we can use sha1 here. but, I didn't use it to keep the URL in the value that allows for viewing and editing the URL directly in the database.

Fair enough.

No specific reason for that. I thought whatever we use, we have to compare it with the value of the same function (site_url() every time). So, I proceed with site_url(). please let me know if I missing anything here and it's better to use home_url() here.

I'd suggest home_url() as the URLs being shared are more closely associated with that than site_url().

@iamdharmesh
Copy link
Member Author

@peterwilsoncc I have made the requested changes in PR, please help to verify once you get some time.

Thanks.

@iamdharmesh iamdharmesh requested a review from peterwilsoncc May 17, 2022 15:04
peterwilsoncc
peterwilsoncc previously approved these changes May 18, 2022
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

I've added a few nitpicks in about commenting and documentation style. All the code is good.

includes/class-ast-staging.php Outdated Show resolved Hide resolved
includes/class-ast-staging.php Outdated Show resolved Hide resolved
includes/admin/post-transition.php Outdated Show resolved Hide resolved
includes/admin/post-transition.php Outdated Show resolved Hide resolved
includes/class-ast-staging.php Outdated Show resolved Hide resolved
includes/class-ast-staging.php Outdated Show resolved Hide resolved
includes/class-ast-staging.php Outdated Show resolved Hide resolved
includes/class-ast-staging.php Outdated Show resolved Hide resolved
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the nitpicks, LGTM.

@jeffpaul jeffpaul merged commit cfbf105 into develop May 19, 2022
@jeffpaul jeffpaul deleted the fix/39 branch May 19, 2022 01:38
@iamdharmesh iamdharmesh modified the milestones: 1.2.0, 1.1.2 Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

TODOs from readme.txt
3 participants