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.

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

Progressive Registration #3833

Closed
maxceem opened this issue Apr 2, 2020 · 35 comments
Closed

Progressive Registration #3833

maxceem opened this issue Apr 2, 2020 · 35 comments
Assignees
Milestone

Comments

@maxceem
Copy link
Collaborator

maxceem commented Apr 2, 2020

This is a new big feature as per design https://marvelapp.com/e0gjig7/screen/67320435

image

@maxceem maxceem added this to the 2.9 milestone Apr 2, 2020
@maxceem
Copy link
Collaborator Author

maxceem commented Apr 2, 2020

@vikasrohit the design for this form introduces the 3rd variation of the design for this form. We already have a registration form:

image

and Profile Information Form:

image

I suggest reusing the design from the Profile Information Form for this new form instead of creating a new variation of design:

image

@vikasrohit
Copy link

I am okay with it for now as I don't see much difference in new design as well. However, please do keep the required vs optional fields requirements as is i.e. use the requirement doc as primary source for making fields required or optional (based on roles).

@coderhacker
Copy link
Collaborator

coderhacker commented Apr 21, 2020

@maxceem
QA testing against "Profile Information": screens https://marvelapp.com/e0gjig7/screen/67320439,
https://marvelapp.com/e0gjig7/screen/67320436,
https://marvelapp.com/e0gjig7/screen/67320437,
https://marvelapp.com/e0gjig7/screen/67320438:

1. Fixed
2. Fixed
3. Fixed
4. Fixed
5 to 12. All labels are not matching with the provided design.
13. Cancel and Sent My Request button is missing
14
image is not matching with provided desing. hyperlink is missing.

image

15. Fixed

image

PS: All validation is working as expected.

CC: @codejamtc @drasticdpk

@appirio-tech appirio-tech deleted a comment from coderhacker Apr 21, 2020
@maxceem
Copy link
Collaborator Author

maxceem commented Apr 21, 2020

@coderhacker we didn't have a task to update the Profile Information form. It should stay same as it is now.

Could you please, ask someone from QA team to share with you the google document with the task description.

@coderhacker
Copy link
Collaborator

@maxceem: I have updated the above comments. There is some minor issue which needs to fix.

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 21, 2020

Thanks, @coderhacker.

5 to 12. All labels are not matching with the provided design.

We reuse our existing form design. So we keep labels on the left, not on the top as on the new design.

  1. Cancel and Sent My Request button is missing

Cancel button was renamed to Back as we use the Back button on all other places. It was confirmed in the Google Document comments (unfortunately, comments are not included in the copy of the document I've shared with you, but there are not many comments).

Sent My Request button is there:
image

  1. Good catch, have to fix it.

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 21, 2020

  1. is fixed now:

    image

@coderhacker could you please let me know if anything else has to be fixed per the clarifications above #3833 (comment)

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 22, 2020

We've found some issue with progressive registration when using a new empty Topcoder/Copilot account. So please, hold on with continue testing, we would fix it first.

cc @codejamtc

@codejamtc
Copy link
Collaborator

codejamtc commented Apr 22, 2020

@maxceem I can see few issues on the form {Client Workflow}: ❌
https://monosnap.com/file/Ivl770NJ9SvMXu4fQO0tSiw0ugJHox

Reference: https://marvelapp.com/e0gjig7/screen/67320436 {Design} | https://docs.google.com/document/d/1M7IXe6Pvz4hEAsKy503_Fdbs6cLq0cYXUqxuWIBdPjI/edit# {Requirement}

  1. Company Name is missing on the form. Existing form My Profile > Profile Information has the Company Name field https://monosnap.com/file/Ic7MGQ7QjdnCDPVngnE1KHZF5hfrAs. New design ref: https://marvelapp.com/e0gjig7/screen/67320436

image

  1. Since we have validations activated Title, Business Email and Business Phone labels are not in-line with the text fields

image

  1. Send My Request button is enabled even though the user filled an invalid email to the Business Email field

image

  1. Send my Request button text should be Send My Request button
    Ref: https://docs.google.com/document/d/1M7IXe6Pvz4hEAsKy503_Fdbs6cLq0cYXUqxuWIBdPjI/edit#

image

  1. After removing a few digits from the Business Phone (+1 7138888888) >> +1 7138888 and tap on any empty place won't show the validation message "Invalid business phone"

image

  1. Some country texts are hidden.

image

  1. I guess we can enter white spaces for the required fields because I can see the Send my Request button is enabled.

image

NOTE: Topcoder and Copilot flow testing is pending

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 23, 2020

  1. Company Name is missing on the form.

Great, catch. - Fixed.✅

  1. Since we have validations activated Title, Business Email and Business Phone labels are not in-line with the text fields

I was existent issue from the User Profile Settings page, but it's fixed now. ✅

  1. Send My Request button is enabled even though the user filled an invalid email to the Business Email field

Fixed ✅

  1. Send my Request button text should be Send My Request button

Fixed ✅

  1. After removing a few digits from the Business Phone (+1 7138888888) >> +1 7138888 and tap on any empty place won't show the validation message "Invalid business phone"

Phone validation indeed works strange: Fixed ✅ by 294425b

phone-validation

  1. Some country texts are hidden.

Fixed ✅ by 294425b

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 23, 2020

  1. I guess we can enter white spaces for the required fields because I can see the Send my Request button is enabled.

image

⚠️ This is existent behaviour for most our forms, we treat multiple spaces as if value is entered. @vikasrohit should we change it and not allowed spaces as a value?

maxceem pushed a commit that referenced this issue Apr 23, 2020
ref issue #3833
- show "Company Name" for customers
- align labels when showing validation errors
- added validation for business email
- fixed "Send My Request" button typo
@maxceem
Copy link
Collaborator Author

maxceem commented Apr 23, 2020

A couple clarifications, @vikasrohit.

NOTE: As I understand the main task is to support users who registers using general Topcoder Registration form https://accounts.topcoder-dev.com/member/registration. FYI it only creates basic_info trait with the next fields:

{
    "handle": "maxceem18",
    "firstName": "Max",
    "lastName": "18",
    "userId": 88771387,
    "status": "ACTIVE",
    "email": "[email protected]"
}
  1. For customers users we might as "business email". As connect_info is not populated during initial registration, the business email would be empty and we can enter any email. As a result user might 2 different emails in 2 traits:

    image

    Is it OK?

  2. Imagine if we register new user using general topcoder registration form. And and after we add some topcoder role so now if such a user creates a new project it would have Topcoder User flow during progressive registration, and would see the next form:

    image

    • we fill this form and send

    • server returns error:

      [The title should be provided, The company name should be provided]
      
    • So Job Title and Company Name are required in connect_info trait, but we don't ask about them in the form.

    Should we add Job Title and Company Name as required fields for Topcoder flow?

@vigneshTheDev
Copy link
Collaborator

@maxceem

  1. After removing a few digits from the Business Phone (+1 7138888888) >> +1 7138888 and tap on any empty place won't show the validation message "Invalid business phone"

I see two kinds of validations for phone number input. 1. Earlier we used to rely on libphonenumber-js library (https://www.npmjs.com/package/libphonenumber-js). 2. And then according to this issue appirio-tech/accounts-app#333 (comment), we've added a Regex based validation same as it's done in member service.
These two ways of validations are mixed up that result in the strange behaviour of business phone input.

We can choose to handle this in two ways:

  1. Rely on the regex based validation solely.

Problems:

  • The country selection based on phone number selection still relies on libphonenumber-js.
    So, when the phone number is not valid according to libphonenumber-js, country cannot be automatically selected.
  • It's possible that the user submits non matching country and phone number.
    Peek 2020-04-24 02-25
  1. Keep the libphonenumber-js based validation along with regex based validation (Recommended)
  • In such a case, UI validation will not be same as member service but it'll be a more stricter validation while still making sure the phone number value is acceptable by the member service.
  • The validation done with libphonenumber-js is more accurate. For example, +1 713888 is valid according to the regex match. But it's actually invalid (Check here: https://numverify.com).

Please let me know what you think.

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 27, 2020

I am not sure why it should happen if we are following the same approach as we have it right now in connect profile page. I mean, I just tested that for user who was registered with member registration or in any way it doesn't have connect_info trait. I was able see the same email, first name and last name (which are the only common fields between the two traits), which are there in basic_info trait, on the profile form. So should be the case here, isn't it?

On the My Profile page we don't show Business Email field:

image

The account email is same to basic_info, this is correct, but it's required during registration, and there is no case when user doesn't have it (I guess). Also, we have a special logic to change it, we don't allow freely changing it. So I guess if put Business Email field there, it's some other email which could be freely updated and it could be different from account one.

image

first name and last name (which are the only common fields between the two traits)

Just in case, we keep these fields the same manually in Connect App, backend doesn't populate them automatically and doesn't require them to be the same. In our form we just have special logic to take them from basic_info and prepopulate these fields, if there is connect_info trait yet.

Lets send these fields in hard coded way. Map Title to the User's role (if possible pick the highest privileged user role) and Company Name to Topcoder.

Yes, can do this.

cc @vikasrohit

@maxceem maxceem closed this as completed Apr 27, 2020
@maxceem maxceem reopened this Apr 27, 2020
@vikasrohit
Copy link

In our form we just have special logic to take them from basic_info and prepopulate these fields, if there is connect_info trait yet.

Lets use this in progressive form as well. I think that solve other concerns as well if update the basic_info trait as well when we are saving connect_info trait (I think we are also doing this on profile page right now).

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 27, 2020

Lets use this in progressive form as well. I think that solve other concerns as well if update the basic_info trait as well when we are saving connect_info trait (I think we are also doing this on profile page right now).

We already reuse this logic here. So we don't have issues with First/Last name.

The only thing I have concerns about is Business Email. We have it in Progressive Registration form, but we don't have it in the profile settings.

@vikasrohit
Copy link

Got it. We can do that same now for Business Email in progressive form as well i.e. read from basic_info if connect_info trait not available and when saving the progressive registration form, update both traits.

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 27, 2020

Got it. We can do that same now for Business Email in progressive form as well i.e. read from basic_info if connect_info trait not available and when saving the progressive registration form, update both traits.

Sure. Should users be allowed just edit it? As on the accounts page we have a special workflow for changing email with a confirmation email:

image

@vikasrohit
Copy link

I think it should not be editable, if the email is already filled, because we still have the email change feature disabled in Connect.

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 27, 2020

Topcoder members indeed cannot edit email in Connect. But customer user can edit email in Connect. But there is a special workflow with email confirmation.

@vikasrohit
Copy link

Lets not allow anyone to edit emails from the progressive registration form. Customers can still go to the regular Accounts and Security page to do that.

maxceem pushed a commit that referenced this issue Apr 28, 2020
Don't ask business email

ref issue #3833
maxceem pushed a commit that referenced this issue Apr 28, 2020
Required fields should accept whitespaces

ref issue #3833
@maxceem maxceem added Aha!:Ready for QA and removed Have A Question Developer asked some question to copilot/manager. labels Apr 28, 2020
@maxceem
Copy link
Collaborator Author

maxceem commented Apr 28, 2020

@codejamtc this is now ready for QA.

Note: that as per #3833 (comment) we show disabled field Business Email with prefilled email for customers.

@vikasrohit
Copy link

we don't show Business Email for customers anymore.

@maxceem I meant the field to be there bot not editable. Is it possible to do that now?

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 28, 2020

@maxceem I meant the field to be there bot not editable. Is it possible to do that now?

Yes, will update.

@maxceem
Copy link
Collaborator Author

maxceem commented Apr 28, 2020

@maxceem I meant the field to be there bot not editable. Is it possible to do that now?

Done, and updated comment #3833 (comment)

@codejamtc
Copy link
Collaborator

@maxceem @vikasrohit

Issue 01: 🆗
Issue 02: 🆗
Issue 03: 🆗
Issue 04: 🆗
Issue 05: 🆗
Issue 06: 🆗
Issue 07: 🆗
Issue 08: 🆗 #3833 (comment) - Lets not allow anyone to edit emails from the progressive registration form. Customers can still go to the regular Accounts and Security page to do that.

https://monosnap.com/file/Wj7ZqdVpyoLUps2jwNZKMexuGZkAkp

Suggestions:

Suggestions 01: Better to change the label as Job Title rather than Title.

image

Suggestions 02: Company name to Company Name

image

Suggestions 03: No validation when Start Time >End Time

image

Suggestions 04: Local Timezone and Normal Working Hours are optional so if user selects any values for that fields user can reset the data without restarting the form again. Should have an option to reset it. (Should be able to delete the current selection)

image

NOTE: Above are suggestions so these things won't directly affect the ticket so I will mark it as Pass

@codejamtc codejamtc added NeedToVerifyByManagerAcc Those issue need to verify from manager or admin acc QA Pass in Dev and removed Aha!:Ready for QA labels Apr 28, 2020
@maxceem
Copy link
Collaborator Author

maxceem commented Apr 29, 2020

Thanks @codejamtc for always checking on other things around. Your suggestions look very reasonable so I moved them to a separate issue for the product team to decide on them #3971

There is only one suggestion which I think we shouldn't implement.

Suggestions 03: No validation when Start Time >End Time

image

We support Start Time >End Time. This means that user works starting in the morning and works the whole day until the late night. For example here is a realistic example:

  • Start Time: 9:00
  • End Time: 1:00

Start Time > End Time - it means that user starts at 9 am in the morning and works until 1 am late night.

@codejamtc
Copy link
Collaborator

@maxceem Agree, make sense 👍

@coderhacker coderhacker added QA Pass in Prod and removed NeedToVerifyByManagerAcc Those issue need to verify from manager or admin acc labels Apr 30, 2020
@coderhacker
Copy link
Collaborator

Verified on production and this is working as expected.

PS: Please create a new user from this form https://accounts.topcoder.com/member/registration
and follow steps in the attached video file.

3833_prod_pass.zip

CC @lakshmiathreya @drasticdpk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants