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
P20-903: Display U13 Warning on Section Creation to USA teachers #58497
P20-903: Display U13 Warning on Section Creation to USA teachers #58497
Conversation
@@ -41,6 +41,7 @@ def current | |||
ai_tutor_access_denied: !!current_user.ai_tutor_access_denied, | |||
has_seen_progress_table_v2_invitation: current_user.has_seen_progress_table_v2_invitation?, | |||
date_progress_table_invitation_last_delayed: current_user.date_progress_table_invitation_last_delayed, | |||
us_state: current_user.us_state, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we currently have us_state
for teachers? I was under the impression we dont retrieve us_state
for teachers but instead will use the school's state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need to look up the school_info associated with the teacher. There is state
attribute on the SchoolInfo model. When @mgc1194 and I were looking at it, we saw values like CO
and Colorado
, so you will need to do some pattern matching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except I think we should make the country detection consistent with all the CAP features.
const showStudentsToSectionPermissionWarning = | ||
inUSA && | ||
currentUser.isTeacher && | ||
experiments.isEnabledAllowingQueryString(experiments.CPA_EXPERIENCE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(inUSA &&
currentUser.isTeacher) ||
experiments.isEnabledAllowingQueryString(experiments.CPA_EXPERIENCE);
Should this be an OR statement? My understanding is that the experiment is a way for us to override the conditions for the banner to appear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @wilkie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Should be an ||
as Mario suggests, I think.
With the &&
it would only show the banner if the cpa_experience
DCDO flag is set (or you set it in the URL or via enableExperiments=
). That flag is not set in production, so it wouldn't show at all. I think the intent is to actually roll this out.
That flag should just force you into a situation where things are acting like they would if you were a targeted user and the entire feature is live.
I think I like updating it to an ||
so it's live on deploy that it can still be forced on for testing via that flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! As always, leaving the code better than when you found it Artem!!
I left one small comment, I´m not sure if you are setting the query parameters somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I do think we should do as Mario suggests with the experiment logic and roll out this warning now that it has its dedicated audience.
Great work!
Links