-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add site_id to the SFCC datasource #353
Conversation
): WP_Error|string { | ||
$client_auth_url = sprintf( '%s/shopper/auth/v1/organizations/%s/oauth2/token', $endpoint, $organization_id ); | ||
$client_credentials = base64_encode( sprintf( '%s:%s', $client_id, $client_secret ) ); | ||
|
||
$client_auth_response = wp_remote_post($client_auth_url, [ | ||
'body' => [ | ||
'grant_type' => 'client_credentials', | ||
'channel_id' => 'RefArch', | ||
'channel_id' => $site_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this was different than the site_id
hardcoded elsewhere of RefArchGlobal
so I've corrected this.
|
||
|
||
// ToDo: Remove this once existing SFCC data sources have been migrated. | ||
if ( empty( $config['service_config']['site_id'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is present to stop existing test sites from breaking. It adds the previous value as a default value. Once these test sites have been migrated, this can be removed.
I've added this as an interim fix to add the site_id
in place with the intention that the internal plan to add migration as a feature would prevent this down the line.
@@ -21,6 +21,8 @@ protected static function get_service_config_schema(): array { | |||
'enable_blocks' => Types::nullable( Types::boolean() ), | |||
'organization_id' => Types::string(), | |||
'shortcode' => Types::string(), | |||
// ToDo: Remove the nullable existing SFCC data sources have been migrated. | |||
'site_id' => Types::nullable( Types::string() ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is set to nullable only until the existing test sites are migrated. After that, the nullable part can be dropped.
Same thing as below, it's an interim fix to add the site_id
in place with the intention that the internal plan to add migration would negate this in the future.
@ingeniumed I'm able to see some on-page errors when I switch sources from
I think it's due to |
onChange={ siteId => { | ||
handleOnChange( 'site_id', siteId ?? '' ); | ||
} } | ||
value={ state.site_id ?? 'RefArchGlobal' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided a default value similar to what we had before of RefArchGlobal
. Figured since there isn't exactly an API to get a list of this, I've left it as a textbox with a helpful value.
Closed this PR, as I realized the fixes I'm doing are going to lead to more work when the migration pathway work goes in. It's better to lay the ground work for that, rather than try to get this in. |
Description
The purpose of this PR is to add the
site_id
to any communications with the SFCC API instead of using the hardcoded value ofRefArchGlobal
.This PR does have 2 temporary pieces of code in it, in order to get
site_id
in place. There is an internal discussion and plan to add migration in place for such changes, to avoid future cases like this.Testing
RefArchGlobal
and allow you to change this as well.Future
Once this goes in, I'll create a follow up PR that'll drop the default value and the nullable as well. Once the existing test sites have been upgrade so that the
site_id
is populated for them, that can then be merged.