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

Show settings link & API Key in Network Admin sites list #583

Merged
merged 24 commits into from Jan 25, 2022
Merged

Show settings link & API Key in Network Admin sites list #583

merged 24 commits into from Jan 25, 2022

Conversation

jblz
Copy link
Contributor

@jblz jblz commented Jan 10, 2022 •
edited

Description

In the Multisite Site Listing Table for each subsite:

  • Include a link to the Parsely Settings page in the "row actions"
  • Include info about the API key to see at-a-glance which subsites need to be configured
Screen.Recording.2022-01-13.at.2.58.06.PM-trimmed.mov

Motivation and Context

This seeks to provide a good multisite management experience (#575)

How Has This Been Tested?

  • Configure a new multisite (can be either subdirectory or subdomain)
  • Configure some subsites
  • Browse to /wp-admin/network/sites.php
    • Note that all sites (including the main site) inform you that the Parse.ly API key is missing
  • Hover over a site and click Parse.ly Settings
  • Populate the API Key field with some test string
  • Return to /wp-admin/network/sites.php
    • Confirm that the API key is reflected in the appropriate column in the table
  • Rinse & repeat -- confirm the interactions are otherwise ideal

@jblz jblz requested a review from a team as a code owner January 10, 2022 14:32
@jblz jblz self-assigned this Jan 10, 2022
jblz
jblz commented Jan 10, 2022
View reviewed changes
wp-parsely.php Outdated Show resolved Hide resolved
@jblz jblz mentioned this pull request Jan 10, 2022
Merged
@jblz
Copy link
Contributor Author

jblz commented Jan 11, 2022 •
edited

SonarCloud says:

Rename class "Network_Admin_Sites_List" to match the regular expression ^[A-Z][a-zA-Z0-9]*$.

I don't think that that regex matches our sniffs.

@jblz jblz changed the title Show settings link in Network Admin sites list Show settings link & API Key in Network Admin sites list Jan 11, 2022
@pauarge pauarge mentioned this pull request Jan 11, 2022
Closed
@pauarge pauarge added this to the 3.2.0 milestone Jan 11, 2022
pauarge
pauarge suggested changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@pauarge pauarge left a comment

Choose a reason for hiding this comment

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

Changes looking great! Left some small comments, but if we fix those and add some tests this is ready to be shipped 🚢

src/UI/class-network-admin-sites-list.php Outdated Show resolved Hide resolved
src/UI/class-network-admin-sites-list.php Outdated Show resolved Hide resolved
src/UI/class-network-admin-sites-list.php Outdated Show resolved Hide resolved
src/UI/class-network-admin-sites-list.php Outdated Show resolved Hide resolved
src/UI/class-network-admin-sites-list.php Outdated Show resolved Hide resolved
GaryJones
GaryJones previously requested changes Jan 11, 2022
View reviewed changes
src/UI/class-network-admin-sites-list.php Show resolved Hide resolved
src/UI/class-network-admin-sites-list.php Outdated Show resolved Hide resolved
src/UI/class-network-admin-sites-list.php Outdated Show resolved Hide resolved
src/UI/class-network-admin-sites-list.php Show resolved Hide resolved
src/class-parsely.php Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jan 11, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jblz jblz force-pushed the add/network-site-list-settings-link branch from 2250242 to 71ad94e Compare January 11, 2022 21:09
pauarge
pauarge previously approved these changes Jan 12, 2022
View reviewed changes
Copy link
Contributor

@pauarge pauarge left a comment

Choose a reason for hiding this comment

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

LGTM

@jblz jblz force-pushed the add/network-site-list-settings-link branch 3 times, most recently from b7e95e4 to e63ddaa Compare January 13, 2022 20:34
@jblz jblz requested a review from GaryJones January 13, 2022 20:49
@jblz jblz requested a review from pauarge January 13, 2022 20:50
pauarge
pauarge suggested changes Jan 17, 2022
View reviewed changes
Copy link
Contributor

@pauarge pauarge left a comment

Choose a reason for hiding this comment

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

Looking super promising. It just needs a small change.

wp-parsely.php Outdated Show resolved Hide resolved
GaryJones
GaryJones previously requested changes Jan 18, 2022
View reviewed changes
Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

Some minor bits.

src/UI/class-network-admin-sites-list.php Outdated Show resolved Hide resolved
tests/Integration/UI/NetworkAdminSitesListTest.php Outdated Show resolved Hide resolved
tests/Integration/UI/NetworkAdminSitesListTest.php Outdated Show resolved Hide resolved
tests/Integration/UI/NetworkAdminSitesListTest.php Outdated Show resolved Hide resolved
@GaryJones
Copy link
Contributor

Wondering if this could be registered to Screen Options, so that folks have the opportunity to hide the column if they don't need the data? They may already have a bunch of other plugin-added columns, and seeing the state of Parse.ly is not their focus.

@jblz jblz force-pushed the add/network-site-list-settings-link branch from 1eb8950 to 73063de Compare January 19, 2022 14:56
pauarge
pauarge previously approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@pauarge pauarge left a comment

Choose a reason for hiding this comment

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

The PR looks good to me, great work! I agree with the minor suggestions that @GaryJones is saying, but other than that we're good to merge.

@jblz
Copy link
Contributor Author

jblz commented Jan 25, 2022

Wondering if this could be registered to Screen Options, so that folks have the opportunity to hide the column if they don't need the data?

Cool! Apparently, the column is already automatically toggle-able via Screen Options:

Screen.Recording.2022-01-25.at.11.27.20.AM.mov

@jblz jblz requested a review from GaryJones January 25, 2022 17:02
pauarge
pauarge approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@pauarge pauarge left a comment

Choose a reason for hiding this comment

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

LGTM

@jblz jblz merged commit 9c930ed into develop Jan 25, 2022
@jblz jblz deleted the add/network-site-list-settings-link branch January 25, 2022 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pauarge pauarge pauarge approved these changes

@GaryJones GaryJones Awaiting requested review from GaryJones

Assignees

@jblz jblz

Labels
None yet
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jblz @GaryJones @pauarge @acicovic