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.

Sign up for GitHub

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

Jump to bottom

Fix NLU Settings and Wp-CLI #96

Merged
merged 16 commits into from
Jul 24, 2019
Merged

Fix NLU Settings and Wp-CLI #96

merged 16 commits into from
Jul 24, 2019

Conversation

JayWood
Copy link
Contributor

@JayWood JayWood commented Jun 27, 2019 •
edited by jeffpaul
Loading

Fixes NLU Settings backwards compatibility and WP-CLI command registration.

Benefits

Possible Drawbacks

New Provider classes will be required to implement the reset_settings() method.

Verification Process

Settings

  1. Installed 1.2.1 - via git checkout 1.2.1
  2. Set up Watson settings
  3. Checked out my branch git checkout bug/nlu-credentials
  4. Traced settings with xDebug to ensure they're set still.

xDebug Screenshot of endpoint results: https://i.gyazo.com/d01608a12c69244cb655bfe67a361781.png

WP-CLI

  1. Checked out my branch git checkout bug/nlu-credentials
  2. Ran wp classifai --help
  3. Verified commands existed
  4. Ran wp classifai reset
  5. Verified settings were reset, leaving licensing intact.

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.

@JayWood
Key services in the array so that they may be pulled and queried later.
921d1fb
@JayWood
Make get_settings public, return all settings for get_plugin_settings.
3640579
@JayWood
Make settings methods public, update get_plugin_settings
39bb2a5 
This updates the get_plugin_settings() function in helpers to allow for
users to pass in a service to get the settings for that service. This
also keys services in the filter to make them easily queryable when
traversing the objects since numerical indicies can be unpredictable.
@JayWood
Working replacements in helpers.php for plugin settings.
3bdd465
@JayWood
Update the update checker code for settings pull.
07b1616
@JayWood
Register classifai CLI command set - fixes #91
c84c898
@JayWood
Allow each provider class to have it's own reset settings method.
fe494ad
@JayWood
Actually run the reset method if possible.
bce2a9c
@JayWood
Fix logic.
08dc40e
@JayWood
Implement methods and require the new reset_settings() method for all…
e98942a 
… providers.
@JayWood
Update CLI comment to be plugin-wide.
32de487
@JayWood
One-line, for CLI specifically.
1e180e7
@JayWood
Use correct method, mis-spelling on my IDE's part....
92854a9
@JayWood
Actually reset NLU settings. Update classifai option set to new format.
1365cc8
@JayWood
Update confirmation message to be globally accurate, not just Watson.
9007082
@JayWood
Resolve PHPcs build issues.
4962ed8
@jeffpaul jeffpaul added this to the 1.4.0 milestone Jun 27, 2019
@jeffpaul jeffpaul added the type:bug Something isn't working. label Jun 27, 2019
@jeffpaul jeffpaul modified the milestones: 1.4.0, 1.3.2 Jul 19, 2019
@adamsilverstein
Copy link

  • Upgrading worked as expected in my testing
  • verified wp-cli commands available

Could use a code review from someone more familiar with the codebase, overall looks fine to me though.

adamsilverstein
adamsilverstein approved these changes Jul 23, 2019
View reviewed changes
Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

LGTM, appreciate a review from @ryanwelcher or @helen

ryanwelcher
ryanwelcher approved these changes Jul 24, 2019
View reviewed changes
Copy link
Contributor

@ryanwelcher ryanwelcher left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@helen helen merged commit 496de51 into develop Jul 24, 2019
@jeffpaul jeffpaul deleted the bug/nlu-credentials-dev branch March 5, 2020 01:05
@ShahAaron ShahAaron mentioned this pull request Oct 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanwelcher ryanwelcher ryanwelcher approved these changes

@adamsilverstein adamsilverstein adamsilverstein approved these changes

Assignees

@JayWood JayWood

Labels
type:bug Something isn't working.
Projects
None yet
Milestone
1.3.2
Development

Successfully merging this pull request may close these issues.

wp cli commands aren't registered NLU credentials issue in v1.3
5 participants
@JayWood @adamsilverstein @ryanwelcher @helen @jeffpaul