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

Wp-solutions-PS0-2289 - Checking plugin installation and active state #37

Merged
merged 19 commits into from
Nov 24, 2024

Conversation

krsomayagi
Copy link
Contributor

Proposed changes

Code is added to verify if the plugins are installed and active

Type of Change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Video

Checklist

  • I have read the CONTRIBUTING doc
  • I have viewed my change in a web-browser
  • Linting and tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

krsomayagi and others added 2 commits November 6, 2024 13:38
* main:
  troubleshooting test log and performance
  clean up wp cli commands with wpcli wrapper method
  add log to test exec commands
  move the intercept statement to beginning
  NPM(deps): Bump @wordpress/api-fetch from 7.10.0 to 7.11.0
@wpscholar wpscholar requested a review from circlecube November 6, 2024 17:15
@circlecube
Copy link
Member

@krsomayagi thanks for submitting. I saw this test wasn't passing in the runner. I merged main into this branch, and I still don't see this new test passing. Can you take another look? Is it passing for you locally?

krsomayagi and others added 6 commits November 7, 2024 10:58
* main:
  add module supports for login and cli command
  adjust tests for test isolation
  pass methods into card component to load plugins endpoint properly and set up better error handling
  reorder cli commands, add expiration var
@krsomayagi
Copy link
Contributor Author

@krsomayagi thanks for submitting. I saw this test wasn't passing in the runner. I merged main into this branch, and I still don't see this new test passing. Can you take another look? Is it passing for you locally?

Sure. Thank you.

krsomayagi and others added 10 commits November 11, 2024 11:28
* main:
  NPM(deps): Bump @wordpress/api-fetch from 7.11.0 to 7.12.0
  NPM Dev(deps-dev): Bump tailwindcss from 3.4.14 to 3.4.15
  cleanup plugins page test with intercept and helper to setCapability
  only check capabilities to determine if plugin page/tab should display
  wait for intercept before checking page for content
  use option update instead of set and reload page to ensure fresh data
…nstall plugin commands do not cause failures when plugin is not present
checks attributes are correct in each situation and installer functions
pls plugin, free plugin, free install install, installed and active, installed and not active
@circlecube
Copy link
Member

@krsomayagi—This was outdated, so I updated it from main and updated your test file (along with some code tweaks). They are now passing, please take a look and let me know if you have any questions.

@krsomayagi
Copy link
Contributor Author

@krsomayagi—This was outdated, so I updated it from main and updated your test file (along with some code tweaks). They are now passing, please take a look and let me know if you have any questions.

@circlecube
Thank you!

Copy link
Member

@circlecube circlecube left a comment

Choose a reason for hiding this comment

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

I'm approving, but would like someone else to take a look since most of this PR ended up being my own code. I've left comments as to the changes in the code. Sorry for all the whitespace/lint cleanup included.

if ( pluginData?.plsProviderName && pluginData?.plsSlug ) {
return `<button
title="${ isInstalled ? 'Activate' : 'Install' } Premium Plugin"
class="nfd-solutions-availble-list-item-button nfd-solutions-${ this.renderNameAsClass(
Copy link
Member

Choose a reason for hiding this comment

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

I added this little method renderNameAsClass to give us unique class names for each plugin (which helps select them in tests). I also update the order of these installer attributes so they are consistent in each block. The rest of the file changes are due to lint whitespace edits.

href="${this.renderCTAUrl(pluginData?.cta?.url)}"
class="nfd-solutions-availble-list-item-button"
>${pluginData?.cta?.text}</a>`;
} else if ( isInstalled ){ // already installed
Copy link
Member

Choose a reason for hiding this comment

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

I removed this isActive but not isInstalled section, and instead use the later conditionals to just check if it is a premium/pls or free plugin. Since those are the more pertinent deciders of what the markup should be. I did file a ticket with the installer module though, since if a plugin is installed and not active, the installer opens up and fails to install since the directory already exists. This change doesn't change the issue, but simplifies the logic in rendering these buttons.

* main:
  update solutions app test
  NPM(deps): Bump @heroicons/react from 2.1.5 to 2.2.0
@circlecube
Copy link
Member

Heres a video of the plugins Plugins and Tools page before this update:
https://github.com/user-attachments/assets/9b7b1541-fb30-4a32-ac51-9dbcef43966d

and one with the update:
https://github.com/user-attachments/assets/140cdb34-2373-4b63-b23e-37ddfed3059a

I updated the button when the plugin is already installed to still include the installer attributes for free or pls pluigns, the installer still needs to be updated so it doesn't fail when a plugin is already installed but inactive, ideally it can detect that it is already installed and just activate and redirect. That is in a different module though and beyond scope of this PR. Here we're at least getting a more clear error message that the directory already exists - before it errored and said the API returned a non-success code.

@circlecube circlecube merged commit e478b76 into main Nov 24, 2024
2 checks passed
@circlecube circlecube deleted the wp-soln-Press02289 branch November 24, 2024 19:55
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.

3 participants