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

Adds "is_managed" key in Parse.ly Integration if integration is managed by VIP #4802

Conversation

mehmoodak
Copy link
Member

@mehmoodak mehmoodak commented Aug 21, 2023

Description

By default we shows a banner on the plugin which let customers know to add credentials but for integration enabled via VIP we don't want to show this banner because if the site is yet to launch then we don't have its domain and without domain we cannot setup Parse.ly credentials so enabling the integration properly is a two step process:

  1. Enable the integration when environment is init so that customers are already aware of Parse.ly and don't have surprises or unexpected behavior during launch.

  2. Configure the integration ( i.e. setup credentials ) after the launch.

Changes Made

  • While forwarding credentials from platform we have added `is_managed' key which will give signal to the plugin that platform is managing this integration so don't show any banner.

Changelog Description

Adds "is_managed" key in Parse.ly Integration if managed by VIP

Pre-review checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally (or has an appropriate fallback).
  • This change works and has been tested on a Go sandbox.
  • This change has relevant unit tests (if applicable).
  • This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

  1. Check out PR
  2. Clone wp-parsely which is ready for v3.9 and wp_parsely_credentials filter (git clone [email protected]:Parsely/wp-parsely.git wp-parsely-3.8)
  3. Add Parse.ly integration config as mentioned in this PR (Adds Parsely as VIP Integration #4660) but have some without config / credentials
  4. Make sure that the Parse.ly warning banner is not appearing on sites that are enabled but does not have credentials

@mehmoodak mehmoodak self-assigned this Aug 21, 2023
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #4802 (ff735d2) into develop (cf0f5e1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             develop    #4802   +/-   ##
==========================================
  Coverage      28.98%   28.98%           
  Complexity      4692     4692           
==========================================
  Files            276      276           
  Lines          20711    20711           
==========================================
  Hits            6004     6004           
  Misses         14707    14707           
Files Changed Coverage Δ
integrations/parsely.php 92.30% <100.00%> (ø)

@mehmoodak mehmoodak force-pushed the vip-parsely-61/adds-is-managed-attribute-on-parsely-integration branch from 2bd16ca to 701ec7e Compare August 21, 2023 08:17
@mehmoodak mehmoodak changed the title Adds "is_managed" key while setting up credentials for Parse.ly Integration Adds "is_managed" key in Parse.ly Integration if integration is managed by VIP Aug 21, 2023
@mehmoodak mehmoodak marked this pull request as ready for review August 21, 2023 08:51
@mehmoodak mehmoodak requested a review from a team as a code owner August 21, 2023 08:51
return array(
// If config provided by VIP is empty then take original credentials else take
// credentials from config.
$credentials = empty( $config ) ? $original_credentials : array(
Copy link
Contributor

@alecgeatches alecgeatches Aug 21, 2023

Choose a reason for hiding this comment

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

Small suggestion - may be better to check for the keys you need explicitly (site_id and api_secret) rather than any $config value. If $config currently or in the future accepts other configuration keys, this code would set both values to null if any other configuration key is set.

Suggested change
$credentials = empty( $config ) ? $original_credentials : array(
$credentials = ( isset( $config[ 'site_id' ] ) && isset( $config[ 'api_secret' ] ) ? array( ... ) : $original_credentials;

(note this implementation switches the order of the array values and $original_credentials)

Copy link
Member Author

Choose a reason for hiding this comment

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

$original_credentials means credentials that we got from plugin so I think its fine to return as it is without any extra checks to keep things simple. Later we can improve if needed.

Copy link
Contributor

@alecgeatches alecgeatches left a comment

Choose a reason for hiding this comment

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

Small suggestion on $config value testing, otherwise good to go!

@mehmoodak mehmoodak merged commit 9e1dd30 into develop Aug 21, 2023
41 checks passed
@mehmoodak mehmoodak deleted the vip-parsely-61/adds-is-managed-attribute-on-parsely-integration branch August 21, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants