Dashboard: Google Analytics id updates via settings view #3777

BrittanyIRL · 2020-08-10T20:53:50Z

Summary

New flag to enableSettingsView that, when enabled, allows a user to update their Google Analytics Id if they have editing capabilities.

Relevant Technical Choices

New flag called enableSettingsView to control settings view visibility.
- moved the settings page out from control of inProgress flagging
Sets up Karma for editorSettings view
Basic front end validation for the proper format of a google analytics id before sending input to API
Connects to settings API

To Do (Amended)

These should be follow up tickets/prs

Currently, the UI is not aware of if sitekit is installed or not per product spec
How to handle success of ID updates. Do we want a success toast?

User-facing changes

When the flag is enabled and you are logged in as an admin, you should be able to navigate to settings from the dashboard and update the google analytics id.

Testing Instructions

Login as an Admin role and enable flag and see the above UI for validation.
Only input formatted as an id (ie: ua-000000-2) or empty input will call API
Only when the input is different from existing value will the API be called
you must hit 'enter' to save the ID update.

Login as any other role besides admin and see that settings is no longer available (this one is hard to test with the experiment flag UI since it doesn't show up - instead for this you can manually update includes/Experiments.php enableSettingsView to 'default' => true (line 242).

Addresses #3482

github-actions · 2020-08-10T20:58:46Z

Size Change: +593 B (0%)

Total Size: 1.17 MB

Filename	Size	Change
`assets/js/edit-story.js`	510 kB	+7 B (0%)
`assets/js/stories-dashboard.js`	576 kB	+586 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	268 B	0 B
`assets/css/stories-dashboard.css`	305 B	0 B
`assets/css/web-stories-embed-block.css`	515 B	0 B
`assets/js/web-stories-activation-notice.js`	63 kB	0 B
`assets/js/web-stories-embed-block.js`	16.7 kB	0 B

_{compressed-size-action}

BrittanyIRL · 2020-08-10T21:33:11Z

assets/src/dashboard/app/views/editorSettings/index.js

+    ({ newGoogleAnalyticsId }) => {
+      const updatedSettings = {
+        googleAnalyticsId:
+          typeof newGoogleAnalyticsId === 'string' && newGoogleAnalyticsId,
+      };


Am checking for a string here to know that it's the analytics ID that is getting updated so that the same handler can be used for publisher logos.

Would a publisher logo be a blob at this point or a string of a path?

it's a blob

and it'll come in as it's own key in the prop object called 'publisherLogo'. Here i'm really checking to see if the update to the ga id is either a) a new id or b) removing an existing id or c) nothing to do with the ga id at all (which would mean it's false)

codecov · 2020-08-10T22:10:36Z

Codecov Report

Merging #3777 into main will increase coverage by 9.42%.
The diff coverage is 91.89%.

@@            Coverage Diff             @@
##             main    #3777      +/-   ##
==========================================
+ Coverage   73.81%   83.24%   +9.42%     
==========================================
  Files          34      787     +753     
  Lines        2108    13695   +11587     
==========================================
+ Hits         1556    11400    +9844     
- Misses        552     2295    +1743

Flag	Coverage Δ
#karmatests	`67.76% <93.75%> (?)`
#unittests	`67.14% <68.75%> (?)`

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

Impacted Files	Coverage Δ
assets/src/dashboard/app/api/useSettingsApi.js	`81.25% <ø> (ø)`
assets/src/dashboard/app/reducer/settings.js	`83.33% <ø> (ø)`
assets/src/dashboard/constants/index.js	`100.00% <ø> (ø)`
assets/src/dashboard/theme.js	`83.33% <ø> (ø)`
includes/Dashboard.php	`22.72% <0.00%> (+1.62%)`	⬆️
assets/src/dashboard/app/views/toaster/index.js	`76.92% <75.00%> (ø)`
...ts/src/dashboard/components/pageStructure/index.js	`90.90% <83.33%> (ø)`
assets/src/dashboard/app/index.js	`100.00% <100.00%> (ø)`
...c/dashboard/app/views/editorSettings/components.js	`90.47% <100.00%> (ø)`
.../app/views/editorSettings/googleAnalytics/index.js	`92.30% <100.00%> (ø)`
... and 774 more

carloskelly13 · 2020-08-11T12:22:13Z

It works well. One product question I have that can be addressed in future PRs since this is feature flagged.

On a succesful ID entry I see no indication it was actually successful, just on failures.

swissspidy · 2020-08-11T12:24:50Z

I don't know what the article is that is supposed to be linked in the helper text (question out to Sam on #2747)

Note that we cannot use markup in translated texts yet, see #1578

So if you add this link, I suggest doing something like this:

TEXT.CONTEXT = __('[...] read this article:', 'web-stories' ); // note the colon
<TextInputHelperText>{TEXT.CONTEXT}</TextInputHelperText>
<TextInputHelperText><a href="https://blog.amp.dev/2019/08/28/analytics-for-your-amp-stories/">{__('Analytics for your Web Stories')}</TextInputHelperText>

Currently, the UI is not aware of if sitekit is installed or not per product spec

I don't think there's UX for this yet. Sounds like something for a follow-up issue?

Passing the information from PHP would be trivial though.

Currently, if a user doesn't have permissions to update the google analytics id they get a 403 forbidden status response and can't see the id at all. Since it's not really an error, just a permission that's not granted I feel like this should just be a response not an error. Think this is something that would benefit from being discussed.

You can check capabilities.canManageSettings beforehand to see whether the user has the necessary permission. I'd simply not show the page if the user doesn't have that permission.

BrittanyIRL · 2020-08-11T15:50:44Z

You can check capabilities.canManageSettings beforehand to see whether the user has the necessary permission. I'd simply not show the page if the user doesn't have that permission.

@swissspidy this sounds great! I am checking for canManageSettings but had been using it to disable the input to update if it's false. I'll move this check up to where we allow the settings page to be a viable route and update the nav options. thanks for the input!

BrittanyIRL · 2020-08-11T15:55:32Z

(converting to draft to make changes suggested by Pascal). Will update for review.

carloskelly13

This looks good

assets/src/dashboard/app/views/editorSettings/googleAnalytics/index.js

maxyinger

lgtm

BrittanyIRL · 2020-08-12T15:52:42Z

@swissspidy When you get a chance - do you have any other change requests here or is it ok to move to ready for QA?

swissspidy

LGTM 👍

spacedmonkey · 2020-08-12T17:41:35Z

When I tested this, it didn't work. I didn't see an update button or any way to save or any request to go to the server. I am missing something?

BrittanyIRL · 2020-08-12T18:01:24Z

When I tested this, it didn't work. I didn't see an update button or any way to save or any request to go to the server. I am missing something?

You have to hit 'enter' - it's the same input style as the 'rename' of a story. I reused the input since we have new designs incoming. I agree there should be some sort of button but since this is feature flagged we can circle back. Try hitting enter and see if it works.

swissspidy · 2020-08-12T18:48:49Z

I agree there should be some sort of button

Accessibility is key here, and should be noted as such on the ticket.

BrittanyIRL · 2020-08-12T18:50:40Z

I agree there should be some sort of button

Accessibility is key here, and should be noted as such on the ticket.

I'll make a new ticket for this and flag it as UX needed so that it doesn't get lost in the shuffle.

#3843

spacedmonkey · 2020-08-13T09:22:28Z

. I'll make a new ticket for this and flag it as UX needed so that it doesn't get lost in the shuffle.

This is a form that submits something, it needs a button for save for accessibility reasons. I didn't think to press enter at all.

carloskelly13 · 2020-08-13T15:46:31Z

@spacedmonkey I 100% agree and when testing this out I was confused too. But I think this is something that is a product decision and so far auto-submit or Enter to submit has been the choice. Definitely something we should flag because so far 3 engineers testing this have all been initially stumped.

Since this is still just in storybook and also behind a flag I think we can merge this in and iterate when we come to a consensus. I'm nervous about letting branches stagnate over product decisions, not technical ones.

…s view to app with route

… settings

…ngs.

…ff to API, update input to show error structure

…ierachy

…ut needs testing before being allowed to be used. prepping settings page to be usable by clearing out the stubbed publisher logo component (that will follow as a separate PR under the same flag). Adjusting the secondary app routes to conditionally allow editor settings to be visible. Have to check flag at app level too to allow SettingsView to be available.

…w flag

… for enabled settings view flag. When both canManageSettins and that flag are true then the settings page is allowed to render. Doing same check in sideNav in app nav links to show or hide the settings page link.

…ly added it to take care of canmanageSettings

google-cla bot added the cla: yes label Aug 10, 2020

BrittanyIRL self-assigned this Aug 10, 2020

BrittanyIRL added Group: Dashboard Pod: Pea (Dashboard & Templates) Type: Enhancement New feature or improvement of an existing feature labels Aug 10, 2020

BrittanyIRL commented Aug 10, 2020

View reviewed changes

BrittanyIRL marked this pull request as ready for review August 10, 2020 22:16

BrittanyIRL requested review from spacedmonkey and swissspidy as code owners August 10, 2020 22:16

BrittanyIRL requested review from dmmulroy, carloskelly13, mariano-formidable and maxyinger August 10, 2020 22:16

BrittanyIRL added this to the Sprint 35 milestone Aug 10, 2020

BrittanyIRL force-pushed the feature/3482-settings-analytics branch from 69e6ff0 to 793ae03 Compare August 10, 2020 23:07

swissspidy removed the request for review from spacedmonkey August 11, 2020 12:17

BrittanyIRL marked this pull request as draft August 11, 2020 15:54

BrittanyIRL removed request for swissspidy, dmmulroy, carloskelly13, mariano-formidable and maxyinger August 11, 2020 15:54

BrittanyIRL changed the title ~~Dashboard: Google Analytics id updates via settings view~~ [DRAFT] Dashboard: Google Analytics id updates via settings view Aug 11, 2020

BrittanyIRL changed the title ~~[DRAFT] Dashboard: Google Analytics id updates via settings view~~ Dashboard: Google Analytics id updates via settings view Aug 11, 2020

carloskelly13 approved these changes Aug 11, 2020

View reviewed changes

swissspidy reviewed Aug 11, 2020

View reviewed changes

assets/src/dashboard/app/views/editorSettings/googleAnalytics/index.js Outdated Show resolved Hide resolved

BrittanyIRL requested a review from swissspidy August 11, 2020 18:48

maxyinger approved these changes Aug 11, 2020

View reviewed changes

swissspidy approved these changes Aug 12, 2020

View reviewed changes

BrittanyIRL added 15 commits August 14, 2020 08:45

ability to update google analytics from settings view. adding setting…

f491f53

…s view to app with route

adding ability for inline input to be disabled. adding unit tests for…

a0d8aa6

… settings

adding karma tests for settings as well as config for karma for setti…

a1f7f0c

…ngs.

basic regex to validate the format of the entered ID before sending o…

f60cda7

…ff to API, update input to show error structure

fixing double form submit while keeping proper tags. fixing heading h…

2eedef4

…ierachy

adding settings errors to toaster conditionally for enableSettingsVie…

3e79e1e

…w flag

clean up for PR and forgotten karma test for errors.

6bde898

more test clean up

e6ea170

one more test clean up

f97af66

update helper text to have link and better formatting for screen reader

805a6e1

gut disabled prop from input as we do not need this here and i had on…

d91058f

…ly added it to take care of canmanageSettings

move regex pattern for validation above function

2d7fdf0

no more period after article link in helper text

ba7794f

BrittanyIRL force-pushed the feature/3482-settings-analytics branch from b7fcade to ba7794f Compare August 14, 2020 15:47

BrittanyIRL merged commit c781ffb into main Aug 14, 2020

BrittanyIRL deleted the feature/3482-settings-analytics branch August 14, 2020 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard: Google Analytics id updates via settings view #3777

Dashboard: Google Analytics id updates via settings view #3777

BrittanyIRL commented Aug 10, 2020 •

edited

Loading

github-actions bot commented Aug 10, 2020 •

edited

Loading

BrittanyIRL Aug 10, 2020

carloskelly13 Aug 11, 2020

BrittanyIRL Aug 11, 2020

BrittanyIRL Aug 11, 2020

codecov bot commented Aug 10, 2020 •

edited

Loading

carloskelly13 commented Aug 11, 2020 •

edited

Loading

swissspidy commented Aug 11, 2020

BrittanyIRL commented Aug 11, 2020

BrittanyIRL commented Aug 11, 2020

carloskelly13 left a comment •

edited

Loading

maxyinger left a comment

BrittanyIRL commented Aug 12, 2020

swissspidy left a comment

spacedmonkey commented Aug 12, 2020

BrittanyIRL commented Aug 12, 2020

swissspidy commented Aug 12, 2020

BrittanyIRL commented Aug 12, 2020 •

edited

Loading

spacedmonkey commented Aug 13, 2020

carloskelly13 commented Aug 13, 2020 •

edited

Loading

Dashboard: Google Analytics id updates via settings view #3777

Dashboard: Google Analytics id updates via settings view #3777

Conversation

BrittanyIRL commented Aug 10, 2020 • edited Loading

Summary

Relevant Technical Choices

To Do (Amended)

User-facing changes

Testing Instructions

github-actions bot commented Aug 10, 2020 • edited Loading

BrittanyIRL Aug 10, 2020

Choose a reason for hiding this comment

carloskelly13 Aug 11, 2020

Choose a reason for hiding this comment

BrittanyIRL Aug 11, 2020

Choose a reason for hiding this comment

BrittanyIRL Aug 11, 2020

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2020 • edited Loading

Codecov Report

carloskelly13 commented Aug 11, 2020 • edited Loading

swissspidy commented Aug 11, 2020

BrittanyIRL commented Aug 11, 2020

BrittanyIRL commented Aug 11, 2020

carloskelly13 left a comment • edited Loading

Choose a reason for hiding this comment

maxyinger left a comment

Choose a reason for hiding this comment

BrittanyIRL commented Aug 12, 2020

swissspidy left a comment

Choose a reason for hiding this comment

spacedmonkey commented Aug 12, 2020

BrittanyIRL commented Aug 12, 2020

swissspidy commented Aug 12, 2020

BrittanyIRL commented Aug 12, 2020 • edited Loading

spacedmonkey commented Aug 13, 2020

carloskelly13 commented Aug 13, 2020 • edited Loading

BrittanyIRL commented Aug 10, 2020 •

edited

Loading

github-actions bot commented Aug 10, 2020 •

edited

Loading

codecov bot commented Aug 10, 2020 •

edited

Loading

carloskelly13 commented Aug 11, 2020 •

edited

Loading

carloskelly13 left a comment •

edited

Loading

BrittanyIRL commented Aug 12, 2020 •

edited

Loading

carloskelly13 commented Aug 13, 2020 •

edited

Loading