Directly Remove Credentials on 401 #966
Conversation
…eturns a WP_Error
…s' into remove-refresh-resources-calls
…ources' into remove-refresh-resources-calls
As init() is no longer automatically invoked when constructing a resource class, this must be called manually in tests.
n7studios
changed the base branch from
main
to
tests-non-inline-forms-setup-resources
…ources' into remove-refresh-resources-calls
…ources' into remove-refresh-resources-calls
composer.json
Outdated
| "license": "GPLv3", | ||
| "require": { | ||
| "convertkit/convertkit-wordpress-libraries": "2.1.1" | ||
| "convertkit/convertkit-wordpress-libraries": "dev-return-refresh-result" |
There was a problem hiding this comment.
This will need to be changed to the published version of the Kit WordPress LIbraries when released e.g. 2.1.2
| } | ||
|
|
||
| // Call parent initialization function. | ||
| parent::init(); |
There was a problem hiding this comment.
Drops the init() call, so refresh() can only occur if called directly by the Plugin.
| parent::init(); | ||
| // Get last query time and existing resources. | ||
| $this->last_queried = get_option( $this->settings_name . '_last_queried' ); | ||
| $this->resources = get_option( $this->settings_name ); |
There was a problem hiding this comment.
Makes certain any existing cached resources are available, if the API isn't available.
| $id | ||
| ) | ||
| ), | ||
| 404 |
There was a problem hiding this comment.
Provides context to the error i.e. a Kit Form ID was specified that either does not exist in the cached resources, is an ID for a legacy form, or truly doesn't exist (the user entered the wrong ID in the shortcode).
| * 1.6.1: Refresh Forms, Landing Pages and Tags data stored in settings, | ||
| * to get new Forms Builder Settings. | ||
| */ | ||
| private function refresh_resources() { |
There was a problem hiding this comment.
Most 401 errors have the context of block_edit, so this isn't likely to be a culprit. However, it's unlikely many users are upgrading from 1.6.1 to 3.1.0 at this stage, so it shouldn't hut to remove this upgrade path.
n7studios
requested review from
a team,
ciccio-kit and
noelherrick
and removed request for
a team
WordPress Playground🚀 Your PR has been built and is ready for testing in WordPress Playground! |
n7studios
changed the base branch from
tests-non-inline-forms-setup-resources
to
main
WordPress Playground🚀 Your PR has been built and is ready for testing in WordPress Playground! |
noelherrick
left a comment
noelherrick
left a comment
There was a problem hiding this comment.
Let's get this out, and if this turns out to be a good solution, let's put the credential deletion logic in all the resource classes refresh methods. If we don't want to put it into ConvertKit_Resource_V4 in convertkit-wordpress-libraries because we define the deletion logic in this repo, we could have another class in this repo that all the resources inherit from that adds the deletion check in the refresh method.
Summary
#964 did not reduce the number of 401 requests made by Plugins. To address this, this PR:
refresh()call, forcibly removing credentials from the Plugin's settings if the failure is a 401ConvertKit_Resource_Forms, removes theConvertKit_Resource_V4init()call and loads cached resources.All calls made to
refresh()from the Plugin are made by the user:Testing
Existing tests pass.
Existing integration tests confirm deletion of tokens when a 401 occurs, but this will need a PR of end to end tests to check that tokens are truly deleted when using the Plugin and a 401 occurs.
Checklist