-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat(form-v2/react-to-angular/1): add switch to angular option #4263
Conversation
frontend/src/features/public-form/components/PublicSwitchEnvMessage.tsx
Outdated
Show resolved
Hide resolved
frontend/src/features/public-form/components/PublicSwitchEnvMessage.tsx
Outdated
Show resolved
Hide resolved
frontend/src/features/workspace/components/AdminSwitchEnvMessage.tsx
Outdated
Show resolved
Hide resolved
frontend/src/features/workspace/components/AdminSwitchEnvMessage.tsx
Outdated
Show resolved
Hide resolved
@@ -15,6 +15,7 @@ export type SetEnvironmentParams = { | |||
|
|||
export const RESPONDENT_COOKIE_OPTIONS = { | |||
httpOnly: false, | |||
maxAge: 31 * 2 * 24 * 60 * 60, // 2 months |
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.
was this what we decided? @timotheeg thoughts? or we can keep this first.
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.
sorry if the issue is unclear. basically the idea is this:
- if you are randomly assigned React, you should stay on React unless you opt for Angular (hence 2 months)
- if you are randomly assigned AngularJS, you should stay on AngularJS for the next 24h (so the rollout % can be increased the next day)
- if you opt for Angular, you should stay on Angular until the relevant rollout % is 100% (e.g. if I opt for Angular on an email mode form, I will stay on Angular for email mode forms until email mode form rollout is 100%, after which I can be on React. if I opt for Angular on storage mode form, I stay on Angular until storage mode is 100%)
is that clearer?
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.
That doesn't sound right 🤔
if you are randomly assigned React, you should stay on React unless you opt for Angular (hence 2 months)
This will cause the percentage to increase in favour of react more than what the rollout threshold suggests.
I think the random assignment should remain as session as we had discussed, and only explicit switches should have an expended expiry.
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.
Also the change here needs more work to fit the logic. RESPONDENT_COOKIE_OPTIONS
is currently applied on the random assignment, so both angular and react would get the 2 month expiry.
maxAge should probably be added to the explicit choice functions rather than the base constant.
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.
oops sorry, I got confused between this and the admin side. my bad. respondent should be session expiry unless explicitly opting for Angular.
This will cause the percentage to increase in favour of react more than what the rollout threshold suggests.
yup, I wrote about this in #4217. I think on the admin side I'd rather have the deviation than have a situation where admins are randomly flipping back and forth between React and Angular. discuss later!
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.
Based on our discussion on cookie expiry dates:
Public respondents
- random assignment:
- react: session cookie
- angular: session cookie
- personal choice - react to angular: 2 months
Admins
- random assignment
- react: 2 months
- angular: session
- personal choice - react to angular: 2 months
- personal choice - angular to react: 2 months
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.
I've removed maxAge
from the base cookie options, and have added new ADMIN_COOKIE_OPTIONS_WITH_EXPIRY
and RESPONDENT_COOKIE_OPTIONS_WITH_EXPIRY
for the personal choice cases, since it's a standard 2 months for both react to angular and vice versa switches.
TODO: Differentiate random assignment env cookie expiry for admins.
Currently admins are on Angular by default, and see React if they opt in. Have left it since this PR only focuses on the personal choice option, to be tackled in #4217
Flow for switching:
|
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.
lgtm
frontend/src/features/public-form/components/PublicSwitchEnvMessage.tsx
Outdated
Show resolved
Hide resolved
frontend/src/features/workspace/components/AdminSwitchEnvMessage.tsx
Outdated
Show resolved
Hide resolved
const ENV_ENDPOINT = '/environment' | ||
const ADMIN_ENDPOINT = '/admin' | ||
|
||
export const publicChooseEnvironment = async (): Promise<UiCookieValues> => { |
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 you think we should create an issue and link a TODO here so we remember to remove all these related code when we go 100% React?
(and everywhere else that is related to these functions)
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.
good idea, will reference in TODOs + have created a new issue to tag it to
Problem
We need to give public respondents and admins a way to opt out of React and revert back to the AngularJS app.
Makes progress on #3933
Solution
This PR implements the inline messages for both public respondents and admins, giving them the option to switch from React to the AngularJS app.
Key changes:
PublicSwitchEnvMessage
for public respondents, andAdminSwitchEnvMessage
for admins/api/v3/forms/environment/(react|angular)
to allow setting of the cookieOther changes:
Breaking Changes
Future PRs
Before & After Screenshots
AFTER:
Admin
Public
Tests
v2-admin-ui cookie
should change fromangular
toreact
. This cookie should expire in 2 months time.v2-respondent-ui cookie
should bereact
. This cookie should expire in 2 months time.