feat: update email preferences copy #3162
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
enhancement
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @masimons, Action: |
masimons
force-pushed
the
ms-email-preference-assign-to-share
branch
from
689aec9 to
66ad49b
Compare
masimons
changed the title
masimons
force-pushed
the
ms-email-preference-assign-to-share
branch
3 times, most recently
from
5365db6 to
e484cee
Compare
TylerHendrickson
approved these changes
Jun 12, 2024
There was a problem hiding this comment.
@masimons TL;DR Approving this because I think the change looks good overall and it's working for me in manual testing. I do think there's some work to do around testing (see below) but I don't know that it needs to hold up this PR.
Regarding tests: we don't currently have any hard rule that says all Vue template changes need automated tests to be accepted. However, I think it's a good idea to try adding them as we go. Ideally, this change would be tested in a MyProfileView.spec.js file.
I checked out this PR branch and tried my hand at writing some tests; a few observations to share (some or all of which you no doubt also encountered):
- Because the
shareTerminologyEnabled()function from thefeatureFlagsmodule is called within the Vue template'sdata()export, we don't have a way to configure a flag value when mounting aVuex.Storeduring test setup. By contrast, inTeamsView.spec.js, thenewTerminologyEnabledfeature flag value is set by configuring acomputedmethod for the view. - Simply creating a
computedhelper method likeshareTerminologyEnabledinMyProfileView.vuewouldn't be all that useful, sincethis.shareTerminologyEnabled()wouldn't be callable from thedata()export. - I'm wondering if the "correct" way to do this would be to perform a more intensive refactoring of the
MyProfileView.vuetemplate, i.e. that utilizes a custom component for the email toggles or else removes the<v-for="pref in prefs">loop in favor of templating each preference toggle explicitly, which would allow us to evaluate the feature flag in the template logic rather than whendata()is executed. - All that said, I was (almost) able to get tests working without making any changes to
MyProfileView.vueby callinglocalStorage.set('featureFlags', 'Mock JSON-serialized feature flag here')(see below). I also think there's a similar approach here that could work by mocking the@/helpers/featureFlagmodule's exportedshareTerminologyEnabled()function, but I wasn't quite able to figure out Vitest's mocking system.- Note: I say "almost" because I wasn't able to identify a good selector to use when defining the
prefNamevariable. Presumably this could be fixed by tweaking theidattributes in the template a bit, e.g. by adding something like:id="pref.key"in the<b-row>element that loops onv-for="pref in prefs".
- Note: I say "almost" because I wasn't able to identify a good selector to use when defining the
Expand for example test file
import {
describe, beforeEach, afterEach, it, expect,
} from 'vitest';
import { createLocalVue, shallowMount } from '@vue/test-utils';
import Vuex from 'vuex';
import MyProfileView from '@/views/MyProfileView.vue';
const localVue = createLocalVue();
localVue.use(Vuex);
let store;
let wrapper;
const stubs = [];
const noOpGetters = {
'users/loggedInUser': () => undefined,
};
const noOpActions = {
'users/updateEmailSubscriptionPreferencesForLoggedInUser': () => {},
};
afterEach(() => {
store = undefined;
wrapper = undefined;
});
describe('MyProfileView.vue', () => {
describe('shareTerminologyEnabled flag is enabled', () => {
const originalSessionStorageFeatureFlags = sessionStorage.getItem('featureFlags');
beforeEach(() => {
sessionStorage.setItem(
'featureFlags',
JSON.stringify({
...(originalSessionStorageFeatureFlags || {}),
shareTerminologyEnabled: true,
}),
);
store = new Vuex.Store({
getters: {
...noOpGetters,
'users/loggedInUser': () => ({
name: 'Someone',
emailPreferences: { GRANT_ASSIGNMENT: 'SUBSCRIBED' },
}),
},
actions: {
...noOpActions,
},
});
afterEach(() => {
sessionStorage.setItem(
'featureFlags',
originalSessionStorageFeatureFlags,
);
});
wrapper = shallowMount(MyProfileView, {
store,
localVue,
stubs,
computed: {
shareTerminologyEnabled: () => true,
},
});
});
it('should present GRANT_ASSIGNMENT email preferences with share terminology', () => {
expect(JSON.parse(window.sessionStorage.getItem('featureFlags'))).toEqual(
{ shareTerminologyEnabled: true },
);
const prefName = wrapper.find('!!!Could not figure out a selector!!!');
expect(prefName.text()).toEqual('Shared Grants');
});
});
});At this point, my feeling is that it is worth having tests, but there's enough to unpack here that it makes more sense to (as you suggested) create a separate chore issue to discuss a path forward and encapsulate this work. Before I totally commit to that, I do want to see if @jeffsmohan has anything to add here, because he's much more familiar with Vue and Vitest than I am.
|
Oh this brings up more feelings for me than expected, lol. 😝 (To be clear, absolutely nothing to do with your approach here, @masimons — I love the timebox and then check in tactic!) The categories of my feelings:
Sorry for the essay! Upshot is, if you wanted to add tests in this PR, I don't think it's too onerous, fft grab the test code above and finesse it just a bit, and you should be good to go. :) |
jeffsmohan
approved these changes
Jun 13, 2024
| key: 'GRANT_ASSIGNMENT', | ||
| description: 'Send me notifications if a grant has been assigned to my USDR Grants team.', | ||
| description: `${shareTerminologyEnabledFlag ? grantShareDescription : grantAssignmentDescription}`, |
There was a problem hiding this comment.
(minor nit, optional and non-blocking) The js format string with the backticks are unnecessary for the name and description here, since we're not really interpolating anything. You can just do e.g. name: shareTerminologyEnabledFlag ? 'Shared Grants' : 'Grants Assignment', ...
There was a problem hiding this comment.
OOPS you are so right. Missed that during cleanup. Thanks for catching.
masimons
force-pushed
the
ms-email-preference-assign-to-share
branch
from
e484cee to
0f90b07
Compare
jeffsmohan
approved these changes
Jun 13, 2024
There was a problem hiding this comment.
Updates lgtm! You'll just need to get things linted so the check passes. Have you got auto-formatting set up for your editor? A bit of info can be found in our docs here, but I'm happy to talk you through it as the docs aren't exactly extensive.
masimons
force-pushed
the
ms-email-preference-assign-to-share
branch
from
0f90b07 to
dda3551
Compare
masimons
force-pushed
the
ms-email-preference-assign-to-share
branch
from
dda3551 to
1b27fe2
Compare
|
@jeffsmohan a) Thank you SO MUCH for the time you invested in your response, and for sharing working tests and great insight. Really appreciate it 🙏 |
|
No problem, I just hope it was more helpful than overwhelming! :P VSCode should be a pretty straightforward linting setup, hopefully. If you install all the recommended workspace extensions, it should Just Work when you save files. But lmk if you run into trouble. And you're correct — merged PRs in this repo get auto-deployed to staging (you can see the GH Action for this here). But it will not deploy to production automatically. The USDR staff run a process, usually on Tue and Thu, to cut a release, do some manual QA, and then trigger a deploy to production. So if your checks are green and GH allows you to merge the PR, you should feel free to merge it whenever! |
jeffsmohan
approved these changes
Jun 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket #3064
Description
This change is part of the Assign-to-Share project. We want the copy within the Email Notifications settings to change if the feature flag
shareTerminologyEnabledis set to true.Screenshots / Demo Video
"Assign" copy:
"Share" copy:
Testing
I verified this change locally.
MyProfileView.vuedoesn't seem to have a corresponding test file yet. I've been on engineering teams where the process in this case would be to create a separate chore to write tests forMyProfileView.vue(especially since the change made in this PR is small). But if the process here is to commit a test without exception, then I may ask to pair briefly with someone on a unit test for this code (I'm not super familiar with Vue.js, and I hit my timebox for getting a test working on my own).Manual steps for reviewer:
Navigate to
My Profile -> Email Notificationsto verify the copy change (theshareTerminologyEnabledFlagwill need to be manually set to false/true in the code to see the change).Automated and Unit Tests
Manual tests for Reviewer
Checklist