P20-57: Add pre-lockdown parent permission modal #58337

artem-vavilov · 2024-05-01T00:37:09Z

Summary

The modal will appear daily for students who are not compliant with the Child Account Policy until parental permission is requested at least once, and while the CPA experience is cpa_all_user_lockout_warning.

Note: Use the parameter ?show_parental_permission_modal to force the display of the modal.

Screenshots

New parent permission request

Resend parent permission request

Update parent permission request

Links

JIRA ticket: P20-57
Feature flag task: P20-880
Related PR: Allows parent permission request from account email if parent created account #54038

…rning`

…e display the modal

apps/src/redux/parentalPermissionRequestReducer.ts

…t-permission-modal

mgc1194 · 2024-05-02T23:31:39Z

apps/src/templates/policy_compliance/ParentalPermissionModal/index.story.tsx

I liked placing the new components under a new directory.
I am also working on some components, however, I placed them under apps/src/templates/ChildAccountPolicy.
I'd like to hear Dayne's opinion on what directory name we should use, and stay consistent.
I don't mind changing my PR and I'd love to hear more about the pattern used here, with index.tsx and _modal.js

It seems pretty reasonable to place CPA components within that existing namespace. The policy_compliance would be named after the controller that supports it. Much like how there is a sessions path and a rubrics path that similarly group components that are backed by those controllers. Hmm... I think all three of these directories are entirely my fault... I just like congruence between controller namespaces and view/component namespaces when possible.

I applied my own opinion there because ultimately we have no actual convention. (nor is there a perfect answer anyway) Seems like another strategy that teacher tools is employing is to namespace them by project feature. (sectionProgressV2) That also makes sense as a logical way to handle the cognitive load of understanding the interactions between different modules and components... especially in cases that might require a coördinated effort across different systems.

And then there are just honestly way too many that just exist in the templates root. Then again, I'm guilty of putting something in the src root rather recently. heh.

I will be taking these considerations in my PRs :) Thank you Willkie for al the context.

mgc1194 · 2024-05-02T23:35:04Z

apps/i18n/common/en_us.json

+  "policyCompliance_parentalPermissionModal_newRequestForm_title": "Heads up! Your account may soon be locked.",
+  "policyCompliance_parentalPermissionModal_newRequestForm_text1": "Starting on {lockoutDate}, **you will need your parent or guardian’s permission to keep your personal login** with Code.org.",
+  "policyCompliance_parentalPermissionModal_newRequestForm_text2": "To avoid having your account locked, please share your parent or guardian’s email address so they can grant you permission.",
+  "policyCompliance_parentalPermissionModal_updateRequestForm_title": "Thanks! We’ve contacted your parent or guardian.",
+  "policyCompliance_parentalPermissionModal_updateRequestForm_text1": "Didn’t receive anything? Update your parent or guardian’s email below or send another request. Note, your account will be locked if we do not receive your parent/guardian’s permission by {lockoutDate}.",
+  "policyCompliance_parentalPermissionModal_updateRequestForm_text2": "You can also provide this email address at any time from [your profile]({profileUrl}).",
+  "policyCompliance_parentalPermissionModal_lastEmailSentAt": "Last email sent at {sendingTime}",


I've already created some strings under the namespace childAccountPolicy_.
Would it be reasonable to stick to that namespace? If you feel strongly about using policyCompliance_ I'm okey switching the strings I've added, but eitherway we should stay consistent.

code-dot-org/apps/i18n/common/en_us.json

Lines 412 to 413 in 9cb8ea1

"childAccountPolicy_CreateSectionsWarning": "If you have students under 13, they may require parental consent to use a personal login. Please go back and use picture password, secret word, or school-managed sections to avoid any interruptions for students under 13.",

"childAccountPolicy_LearnMore": "Learn more about parental consent",

I think if your ChildAccountPolicy component were in the policy_compliance path, a good name for the string key would then be policyCompliance_childAccountPolicy_learnMore, etc. I generally didn't use an _ when I would do this... that seems nicer though.

That said, I don't think I care too much that the actual name of the component is there... just the major grouping. These strings end up moving around as things are updated (or reused elsewhere) that they shouldn't be too tied to the component name, in my opinion, and be more focused on their relationship to their function. So, for the LockoutPanel, which I put within the sessions grouping because at the time it was supported by the sessions controller, I think I made strings like sessionLockoutNewAccountHeader, etc. So, I expect this to be used by stuff within the sessions path, so session... they have to do with the "Lockout" function... and then the general key.

If I was redesigning everything from scratch, I'd probably want to define the strings in the components themselves and then pull them out instead of trying to define every string in one file. Haha what can you do though.

mgc1194 · 2024-05-02T23:38:06Z

apps/src/templates/sessions/LockoutPanel.jsx

+import headerImage from '@cdo/static/common_images/penguin/yelling.png';
+import headerThanksImage from '@cdo/static/common_images/penguin/dancing.png';


I'm very pro this change.

It makes sense to have the images be near the components that they are used uniquely within and then update them when the images are used in a second container by moving them to the common path.

I am considering namespacing this path so is at least slightly clearer what the context of these images are, but maybe that's not too necessary. A common path just becomes a swamp of random images... which might be nice to avoid. Not a very important problem, though.

mgc1194 · 2024-05-02T23:40:44Z

apps/src/util/currentLocale.ts

+import {get} from 'js-cookie';
+import {DefaultLocale} from '@cdo/generated-scripts/sharedConstants';
+
+export default () => get('language_') || DefaultLocale;


Does this mean we are not surfacing the locale anywhere else in the front end?
This will also allow tacking client-side events with language data.

We do in the homepage/script data and appOptions.localeCode often via the localesRedux which this kind of replaces by looking at the language_ cookie instead of the locale set within the rendered page. It might be nice to update the redux instead to use the default locale on its initial state (I think it is hard-coded there) and then update the redux to setLocaleCode to the cookie value.

The one complication that I'm not fully thinking through is that the locale on the ruby side is in the form en-US and on the JS side it is en_us. Not sure how much that matters in different situations.

window.appOptions are set only on lab pages:

mgc1194 · 2024-05-02T23:48:37Z

dashboard/config/routes.rb

    # Policy Compliance
-    get '/policy_compliance/child_account_consent/', to:
-      'policy_compliance#child_account_consent'
-    post '/policy_compliance/child_account_consent/', to:
-      'policy_compliance#child_account_consent_request'
+    namespace :policy_compliance do
+      get :pending_permission_request
+      get :child_account_consent
+      post :child_account_consent, action: :child_account_consent_request
+    end


I see why you have been using the policy_compliance namespace. I think the Child Account Policy is definitely a type of policy compliance, so it is FERPA or GDPR.
I'm not sure what would be the best term to use across all components @daynew.

One option could be doing a sub-namespace like:

namespace :policy_compliance do namespace :child_accounts get :pending_permission_request get :consent post :consent, action: :consent_request end namespace :gdpr get :information_request post :deletion_request end end

But since we don't actually seem to have any GDPR or other complaince-related routes, it might be YAGNI.

mgc1194 · 2024-05-02T23:51:52Z

dashboard/test/lib/test_cpa.rb

+      Cpa::NEW_USER_LOCKOUT => '2023-01-01T00:00:00Z',
+      Cpa::ALL_USER_LOCKOUT_WARNING => '2023-01-02T00:00:00Z',
+      Cpa::ALL_USER_LOCKOUT => '2023-01-03T00:00:00Z'


Just a note for all reviewers, I'm not sure what the scheduled dates are, but we all need to make sure the dates are accurate. I'm seeing 2023 and I want to make sure it is not supposed to date in 2024.

These are in the tests and they just need to be more or less in an order. This is the proper order. Seems good to have them be a set of dates that is easy to think about and understand quickly.

mgc1194 · 2024-05-02T23:55:08Z

dashboard/lib/policies/child_account.rb

+  # The maximum number of daily requests a student can send to their parent.
+  MAX_STUDENT_DAILY_PARENT_PERMISSION_REQUESTS = 3
+
+  # The maximum number of times a student can resend a request to a parent.
+  MAX_PARENT_PERMISSION_RESENDS = 3
+


Cool! I was wondering if there was something preventing students from spamming with permission requests.

dashboard/lib/cpa.rb

dashboard/config/locales/en.yml

mgc1194

I left a few comments I'd like to address before giving approval.
I addressed everything I'm familiar with but this is a big PR with a lot of important changes and optimizations, so I would still want some more senior engineer to review the changes.

wilkie

Took me a good while to review as it is touching a lot of different things, so apologies.

Looks really really good. I like the adoption of the Forms namespace and I like the cleanup around the code that is dealing with those STATE_POLICY constants.

The modal itself looks very well-engineered. Good work!

…t-permission-modal

carl-codeorg

daynew

This was a lot of work, thank you! Having all the tests written really helped me be confident it works.

daynew · 2024-05-07T23:30:12Z

apps/src/templates/policy_compliance/ParentalPermissionModal/index.tsx

+  const prevParentalPermissionRequest = usePrevious(parentalPermissionRequest);
+
+  const reportEvent = (eventName: string, payload: object = {}) => {
+    analyticsReporter.sendEvent(eventName, payload, PLATFORMS.STATSIG);


Did @alivira request we send these events to Statsig?

You are right, it should be Amplitude only

code-dot-org/apps/src/templates/policy_compliance/ParentalPermissionModal/index.tsx

Lines 58 to 60 in 6b83cf1

const reportEvent = (eventName: string, payload: object = {}) => {

analyticsReporter.sendEvent(eventName, payload, PLATFORMS.AMPLITUDE);

};

dashboard/lib/cpa.rb

daynew · 2024-05-09T23:17:21Z

Could you also update CPA README with any query string params and DCDO configs you added?

artem-vavilov requested a review from a team as a code owner May 1, 2024 00:37

artem-vavilov marked this pull request as draft May 1, 2024 00:37

artem-vavilov force-pushed the P20-57/pre-lockdown-parrent-permission-modal branch 2 times, most recently from 1f1a7b0 to 9d3e4e6 Compare May 1, 2024 01:10

artem-vavilov added 3 commits May 1, 2024 17:42

P20-57: Add pre-lockdown parent permission modal

35954d5

P20-57: Hide the modal behind CPA experience `cpa_all_user_lockout_wa…

90cd18e

…rning`

P20-57: Display modal when parent permission is not requested

5a6fd0c

artem-vavilov force-pushed the P20-57/pre-lockdown-parrent-permission-modal branch from 9d3e4e6 to 5a6fd0c Compare May 1, 2024 14:42

artem-vavilov added 2 commits May 1, 2024 18:15

P20-57: Use ChildAccount namespace

a6d42ee

P20-57: Rename reducer

eda3c1f

artem-vavilov force-pushed the P20-57/pre-lockdown-parrent-permission-modal branch from d3b62dc to eda3c1f Compare May 1, 2024 16:34

artem-vavilov requested a review from a team May 1, 2024 17:33

P20-57: Add param show_pre_lockdown_parent_permission_modal to forc…

117349e

…e display the modal

artem-vavilov force-pushed the P20-57/pre-lockdown-parrent-permission-modal branch from 30a98c1 to 117349e Compare May 1, 2024 17:50

artem-vavilov marked this pull request as ready for review May 1, 2024 17:50

P20-57: Add i18n corrections

6558300

carl-codeorg reviewed May 1, 2024

View reviewed changes

apps/src/redux/parentalPermissionRequestReducer.ts Outdated Show resolved Hide resolved

P20-57: Replace promises with async/await in reducer

8451021

artem-vavilov requested a review from carl-codeorg May 1, 2024 20:09

artem-vavilov added 5 commits May 2, 2024 18:53

P20-57: Rename component

854f205

P20-57: Small fix

fa4e836

Merge remote-tracking branch 'origin' into P20-57/pre-lockdown-parren…

d552e10

…t-permission-modal

P20-57: Fix currentLocale

33fd338

P20-57: Fix apps/src/util/locale-do-not-import.js

36ce9b1

mgc1194 reviewed May 2, 2024

View reviewed changes

dashboard/lib/cpa.rb Outdated Show resolved Hide resolved

mgc1194 reviewed May 3, 2024

View reviewed changes

dashboard/config/locales/en.yml Outdated Show resolved Hide resolved

mgc1194 reviewed May 3, 2024

View reviewed changes

wilkie approved these changes May 3, 2024

View reviewed changes

P20-57: Sort i18n strings alphabetically

1b9b01c

artem-vavilov requested a review from daynew May 3, 2024 12:46

artem-vavilov added 3 commits May 3, 2024 16:36

P20-57: Fix Cpa.cpa_experience desc

dd2dea5

P20-57: Improve component file structure

84d4e4a

Merge remote-tracking branch 'origin' into P20-57/pre-lockdown-parren…

1d4c211

…t-permission-modal

artem-vavilov mentioned this pull request May 6, 2024

P20-119: Add pre-lockdown parental permission banner #58434

Merged

Merge remote-tracking branch 'origin' into P20-57/pre-lockdown-parren…

cef7515

…t-permission-modal

carl-codeorg approved these changes May 8, 2024

View reviewed changes

daynew approved these changes May 9, 2024

View reviewed changes

P20-57: Report events to Amplitude instead of Statsig

0bb37cd

P20-57: Update CPA module doc

6b83cf1

artem-vavilov merged commit f2c3403 into staging May 10, 2024
2 checks passed

artem-vavilov deleted the P20-57/pre-lockdown-parrent-permission-modal branch May 10, 2024 01:09

artem-vavilov mentioned this pull request May 15, 2024

P20-57: Skip CAP compliance check for students whose birthdays are not set #58645

Merged

artem-vavilov mentioned this pull request May 24, 2024

Update CAP Amplitude Events to use CAP namespace #58913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

P20-57: Add pre-lockdown parent permission modal #58337

P20-57: Add pre-lockdown parent permission modal #58337

artem-vavilov commented May 1, 2024 •

edited

mgc1194 May 2, 2024

wilkie May 3, 2024

mgc1194 May 3, 2024

mgc1194 May 2, 2024 •

edited

wilkie May 3, 2024

mgc1194 May 2, 2024

wilkie May 3, 2024

mgc1194 May 2, 2024

wilkie May 3, 2024 •

edited

artem-vavilov May 3, 2024

mgc1194 May 2, 2024

carl-codeorg May 8, 2024

mgc1194 May 2, 2024

wilkie May 3, 2024

mgc1194 May 2, 2024

mgc1194 left a comment

wilkie left a comment

carl-codeorg left a comment

daynew left a comment

daynew May 7, 2024

artem-vavilov May 9, 2024

artem-vavilov May 9, 2024

daynew commented May 9, 2024

	"childAccountPolicy_CreateSectionsWarning": "If you have students under 13, they may require parental consent to use a personal login. Please go back and use picture password, secret word, or school-managed sections to avoid any interruptions for students under 13.",
	"childAccountPolicy_LearnMore": "Learn more about parental consent",

		import headerImage from '@cdo/static/common_images/penguin/yelling.png';
		import headerThanksImage from '@cdo/static/common_images/penguin/dancing.png';

	const reportEvent = (eventName: string, payload: object = {}) => {
	analyticsReporter.sendEvent(eventName, payload, PLATFORMS.AMPLITUDE);
	};

P20-57: Add pre-lockdown parent permission modal #58337

P20-57: Add pre-lockdown parent permission modal #58337

Conversation

artem-vavilov commented May 1, 2024 • edited

Summary

Screenshots

New parent permission request

Resend parent permission request

Update parent permission request

Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgc1194 May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilkie May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgc1194 left a comment

Choose a reason for hiding this comment

wilkie left a comment

Choose a reason for hiding this comment

carl-codeorg left a comment

Choose a reason for hiding this comment

daynew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daynew commented May 9, 2024

artem-vavilov commented May 1, 2024 •

edited

mgc1194 May 2, 2024 •

edited

wilkie May 3, 2024 •

edited