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

Adds streamlined support request flow #6147

Merged
merged 382 commits into from
Sep 13, 2021

Conversation

gagan0123
Copy link
Collaborator

@gagan0123 gagan0123 commented Apr 29, 2021

Summary

This PR adds a new submenu page for AMP that enables the users to easily provide necessary diagnostic data to AMP support team, needed for debugging AMP validation errors they are facing on their sites.

WordPress Dashboard ➞ AMP-WP Menu ➞ Support (new submenu page)

It will enable the user to send the data directly from:

  1. AMP-WP Menu ➞ Support submenu page
  2. AMP admin menu dropdown on frontend.
  3. Edit post screen (in the AMP plugin sidebar)
  4. Plugin row action
  5. Validated URL screen

Before sending the data, it shows to the user what data is exactly going to be sent.

Once the data is sent, the user will be presented with a UUID that they can copy and provide it to support representative on the support request for further assistance.

Closes: #5939

Screenshots:

Support submenu page

Support submenu page

Data preview on the support page

Data preview on the support page

Edit post screen (in the AMP plugin sidebar)

Edit post screen

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@CLAassistant
Copy link

CLAassistant commented Apr 29, 2021

CLA assistant check
All committers have signed the CLA.

@gagan0123 gagan0123 requested a review from westonruter April 29, 2021 17:36
@codecov
Copy link

codecov bot commented Apr 29, 2021

Codecov Report

Merging #6147 (4e2fa1f) into develop (aec975b) will decrease coverage by 2.31%.
The diff coverage is 1.71%.

❗ Current head 4e2fa1f differs from pull request most recent head e593870. Consider uploading reports for the commit e593870 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #6147      +/-   ##
=============================================
- Coverage      75.11%   72.80%   -2.32%     
- Complexity      5858     6046     +188     
=============================================
  Files            234      236       +2     
  Lines          17713    18286     +573     
=============================================
+ Hits           13306    13313       +7     
- Misses          4407     4973     +566     
Flag Coverage Δ Complexity Δ
javascript 79.84% <80.00%> (+<0.01%) 0.00 <0.00> (ø)
php 72.49% <1.03%> (-2.42%) 6046.00 <189.00> (+188.00) ⬇️
unit 72.49% <1.03%> (-2.42%) 6046.00 <189.00> (+188.00) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
amp.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...nents/amp-validation-status/status-notification.js 100.00% <ø> (ø) 0.00 <0.00> (ø)
includes/admin/class-amp-admin-support.php 0.00% <0.00%> (ø) 48.00 <48.00> (?)
includes/admin/class-amp-prepare-data.php 0.00% <0.00%> (ø) 141.00 <141.00> (?)
includes/admin/functions.php 26.31% <0.00%> (-1.99%) 0.00 <0.00> (ø)
...s/validation/class-amp-validated-url-post-type.php 65.27% <ø> (ø) 441.00 <0.00> (ø)
src/Admin/PluginRowMeta.php 100.00% <ø> (ø) 4.00 <0.00> (ø)
assets/src/block-validation/store/index.js 97.82% <75.00%> (-2.18%) 0.00 <0.00> (ø)
...dation/hooks/use-validation-error-state-updates.js 85.29% <100.00%> (+0.21%) 0.00 <0.00> (ø)
includes/amp-helper-functions.php 85.13% <100.00%> (+0.02%) 0.00 <0.00> (ø)
... and 4 more

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Sorry for the prolonged delay with reviewing this PR! It will probably make the most sense for me or @pierlon to refactor the logic in class-amp-admin-support.php and class-amp-prepare-data.php to make use of the new services architecture.

Also, @jwold please review the UX.

@westonruter westonruter requested a review from jwold June 7, 2021 20:24
Copy link
Collaborator

@jwold jwold left a comment

Choose a reason for hiding this comment

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

Great work. I like the changes with the UX.

@westonruter westonruter added this to the v2.2 milestone Jun 22, 2021
@dhaval-parekh dhaval-parekh requested a review from jwold June 22, 2021 18:05
@dhaval-parekh dhaval-parekh self-assigned this Jul 21, 2021
@dhaval-parekh dhaval-parekh requested a review from delawski August 25, 2021 10:56
Copy link
Collaborator

@delawski delawski left a comment

Choose a reason for hiding this comment

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

@dhaval-parekh Thanks for addressing the feedback so far. I've followed up on some comments, so please have a look.

Also, when testing the Support page live, I've noticed some visual issues:

Screenshot 2021-08-25 at 15 49 58

  1. The main box (Selectable) doesn't fit into the viewport and overflows to the right.
  2. The "Create support topic" button doesn't see me to centered vertically with the button.
  3. In case there are no elements in a section (e.g. "Errors (0)"), I don't think there should be an expander next to it (since the expanded pane is empty).

@dhaval-parekh dhaval-parekh requested a review from delawski August 25, 2021 16:34
Copy link
Collaborator

@delawski delawski left a comment

Choose a reason for hiding this comment

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

@dhaval-parekh Thank you for addressing my previous feedback. I left a few more comments but they are very minor.

@dhaval-parekh dhaval-parekh requested a review from delawski August 26, 2021 11:47
Copy link
Collaborator

@delawski delawski left a comment

Choose a reason for hiding this comment

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

@dhaval-parekh From my point of view, once the last issue is addressed, the PR is good to go front-end wise. Thank you for your effort!

@pierlon @westonruter Can you have another look when you get a chance?

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Sorry for the long-overdue review. I'm requesting changes, but I'm going to go ahead and merge since they can be actioned upon in a new PR.

*/
public static function is_needed() {

return current_user_can( 'manage_options' );
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised the service doesn't need to be Delayed. I would have thought that this would be too early for the user to be logged-in.

@westonruter westonruter merged commit a9b5fc2 into ampproject:develop Sep 13, 2021
westonruter added a commit that referenced this pull request Sep 13, 2021
@westonruter westonruter mentioned this pull request Sep 13, 2021
2 tasks
westonruter added a commit that referenced this pull request Sep 14, 2021
westonruter added a commit that referenced this pull request Sep 14, 2021
…detect-page-caching

* 'develop' of github.com:ampproject/amp-wp: (410 commits)
  Fix tests which presumed Hello Dolly was installed
  Fix logic inversion for admin bar item check
  Fix phpstan issues after merging #6147
  Improve prop name and description
  Try fixing failing E2E test
  Fix the unit test
  Update `users` resource prop and its description
  Update assets/src/settings-page/developer-tools.js
  Set current screen to fix test_enqueue_assets_right_hook_suffix
  Enable mobile redirection by default
  Add missing tests for OptionsMenu
  Update param description
  Fix typo
  Allow user-related changes on Options page
  Improve dev tools toggle description
  Update Jest snapshot
  Apply suggestions from code review
  Add E2E test for dev tools toggle
  Fix E2E test
  Fix Other settings box spacing
  ...
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support request flow
9 participants