Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: simplify registration experiment #1164

Merged
merged 3 commits into from
Feb 26, 2024
Merged

feat: simplify registration experiment #1164

merged 3 commits into from
Feb 26, 2024

Conversation

syedsajjadkazmii
Copy link
Contributor

@syedsajjadkazmii syedsajjadkazmii commented Feb 22, 2024 •
edited
Loading

Description

Adds a simplified registration experiment to authn registration page by moving the username field to the second step in the register page for users falling into variation.

JIRA

VAN-1827

How Has This Been Tested?

Locally

Screenshots/sandbox (optional):

Default Variation

Screen.Recording.2024-02-26.at.1.46.57.PM.mov

Simplified Registration variation

Screen.Recording.2024-02-26.at.1.45.24.PM.mov

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Sandbox, if applicable.
  • Is there adequate test coverage for your changes?

Post-merge Checklist

  • Deploy the changes to prod after verifying on stage or ask @openedx/vanguards to do it.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@syedsajjadkazmii syedsajjadkazmii requested a review from a team as a code owner February 22, 2024 12:44
@syedsajjadkazmii syedsajjadkazmii force-pushed the sajjad/VAN-1827 branch 6 times, most recently from 2a66d7b to b41edcc Compare February 26, 2024 08:42
@codecov Codecov
Copy link

codecov bot commented Feb 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 48.09160% with 68 lines in your changes are missing coverage. Please review.

Project coverage is 85.20%. Comparing base (e617a3b) to head (48e6b62).

❗ Current head 48e6b62 differs from pull request most recent head dbe8425. Consider uploading reports for the commit dbe8425 to get more accurate results

Files Patch % Lines
src/register/data/optimizelyExperiment/helper.js 36.11% 20 Missing and 3 partials ⚠️
...ent/useSimplifyRegistrationExperimentVariation.jsx 0.00% 19 Missing ⚠️
src/register/RegistrationPage.jsx 64.86% 13 Missing ⚠️
src/register/data/reducers.js 20.00% 4 Missing ⚠️
src/logistration/Logistration.jsx 66.66% 3 Missing ⚠️
src/register/data/optimizelyExperiment/track.js 57.14% 3 Missing ⚠️
src/register/components/RegistrationFailure.jsx 60.00% 2 Missing ⚠️
src/config/index.js 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1164      +/-   ##
==========================================
- Coverage   87.47%   85.20%   -2.27%     
==========================================
  Files         124      127       +3     
  Lines        2251     2359     +108     
  Branches      629      668      +39     
==========================================
+ Hits         1969     2010      +41     
- Misses        273      337      +64     
- Partials        9       12       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syedsajjadkazmii syedsajjadkazmii force-pushed the sajjad/VAN-1827 branch 2 times, most recently from ee3a1ea to 2490707 Compare February 26, 2024 10:00
mubbsharanwar
mubbsharanwar approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@mubbsharanwar mubbsharanwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but have some comments.

src/register/data/optimizelyExperiment/helper.js
let payload = { ...initPayload };

// Removing username field that is in second step of registration form
delete payload.username;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are deleting the username from the payload, how are we handling this use case, calling API without a username?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just for validating complete first step of simplified register page. On first step, we don't have username so we are not validating it in the first step. We are validating username in the second step.

src/register/data/utils.js Outdated
@@ -35,6 +35,7 @@ export const isFormValid = (
) => {
const fieldErrors = { ...errors };
let isValid = true;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@syedsajjadkazmii syedsajjadkazmii merged commit 9f8a1af into master Feb 26, 2024
5 checks passed
@syedsajjadkazmii syedsajjadkazmii deleted the sajjad/VAN-1827 branch February 26, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahtesham-quraish ahtesham-quraish ahtesham-quraish approved these changes

@mubbsharanwar mubbsharanwar mubbsharanwar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@syedsajjadkazmii @ahtesham-quraish @mubbsharanwar