- 
                Notifications
    
You must be signed in to change notification settings  - Fork 32
 
✨ [Frontend] Expose phone number #8260
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
✨ [Frontend] Expose phone number #8260
Conversation
          
🧪 CI InsightsHere's what we observed from your CI run for 4f2c158. 🟢 All jobs passed!But CI Insights is watching 👀  | 
    
…arc-simcore into feature/change-phone-number
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.
Pull Request Overview
This PR adds phone number support to the frontend, particularly for 2FA SMS functionality. It refactors the handling of "everyone" groups to use centralized methods and conditionally enables SMS 2FA based on whether a user has a phone number set.
- Refactors hardcoded "everyone" group ID arrays to use centralized methods
 - Adds phone number field to user profile when 2FA is required
 - Conditionally enables SMS 2FA option based on phone number availability
 
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description | 
|---|---|
| osparc/ui/list/CollaboratorListItem.js | Replaces inline array with centralized method for getting everyone group IDs | 
| osparc/store/Groups.js | Adds null safety checks and new methods for getting everyone groups/IDs | 
| osparc/share/Collaborators.js | Refactors to use centralized everyone group ID method | 
| osparc/desktop/account/ProfilePage.js | Adds phone number field and conditionally enables SMS 2FA | 
| osparc/data/model/User.js | Adds phoneNumber property to User model | 
| osparc/dashboard/StudyBrowser.js | Updates template filtering to use centralized everyone group methods | 
| osparc/dashboard/CardBase.js | Refactors sharing logic to use centralized everyone group methods | 
          
 | 
    
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.
thx
suggestions:
- As a user, if I see the SMS disabled, i am not sure why and/or how to enable it. Instead of disabling SMS, why not prompt to enter a phone number if selected?
- alternatively add a note on hover that says that it gets enabled when valid phone is provided
 
 - If no phone, i would add background text help.
 - Info icon with doc on hover: e.g. valid phone number used for 2FA etc ...
 
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.
This must be a dev-feature i.e. can only show and be used if DEV_FEATURE is active
          
 @pcrespov Why is this a dev-feature?  | 
    
          
 We discussed it yesterday :-) The reason is that there is one part missing  | 
    
          
 @pcrespov This is about showing it, no updating.  | 
    
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.
thx





What do these changes do?
This PR exposes phone number to the frontend, particularly for 2FA SMS functionality and conditionally enables SMS 2FA based on whether a user has a phone number set.
It also refactors the handling of "everyone" groups to use centralized methods
Next step: let users update their phone number.
Without a Phone Number set:

With Phone Number set:

Related issue/s
How to test
Dev-ops