Skip to content

✨ [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.

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
Merged
merged 12 commits into from
Aug 26, 2025
Merged

✨ [Frontend] Expose phone number #8260

merged 12 commits into from
Aug 26, 2025

Conversation

@odeimaiz
Copy link
Member

@odeimaiz odeimaiz commented Aug 25, 2025 •
edited by pcrespov
Loading

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:
NoPhone

With Phone Number set:
YesPhone

Related issue/s

How to test

Dev-ops

@odeimaiz odeimaiz self-assigned this Aug 25, 2025
@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2025 •
edited
Loading

🧪 CI Insights

Here's what we observed from your CI run for 4f2c158.

🟢 All jobs passed!

But CI Insights is watching 👀

@odeimaiz odeimaiz added t:enhancement Improvement or request on an existing feature bug buggy, it does not work as expected a:frontend issue affecting the front-end (area group) labels Aug 26, 2025
@odeimaiz odeimaiz added this to the Voyager milestone Aug 26, 2025
@odeimaiz odeimaiz marked this pull request as ready for review August 26, 2025 08:04
@odeimaiz odeimaiz requested a review from Copilot August 26, 2025 08:06
Copilot AI reviewed Aug 26, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a 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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 26, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

giancarloromeo
giancarloromeo reviewed Aug 26, 2025
View reviewed changes
Copy link
Contributor

@giancarloromeo giancarloromeo left a comment

Choose a reason for hiding this comment

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

☎️

pcrespov
pcrespov approved these changes Aug 26, 2025
View reviewed changes
Copy link
Member

@pcrespov pcrespov left a comment •
edited
Loading

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 ...

pcrespov
pcrespov requested changes Aug 26, 2025
View reviewed changes
Copy link
Member

@pcrespov pcrespov left a comment •
edited
Loading

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

@odeimaiz
Copy link
Member Author

odeimaiz commented Aug 26, 2025

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?

@odeimaiz odeimaiz requested a review from pcrespov August 26, 2025 08:28
@pcrespov
Copy link
Member

pcrespov commented Aug 26, 2025

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 :-)
My PR #8106 marked the API endpoints as DEV_FEATURE, i.e. they will return NotImplemented error if this flag is off
image

The reason is that there is one part missing
I guess you can show the telephone (i.e. read) but cannot edit it (i.e. write) if DEV_FEATURE is not enabled

@odeimaiz
Copy link
Member Author

odeimaiz commented Aug 26, 2025 •
edited
Loading

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 :-) My PR #8106 marked the API endpoints as DEV_FEATURE, i.e. they will return NotImplemented error if this flag is off image

The reason is that there is one part missing I guess you can show the telephone (i.e. read) but cannot edit it (i.e. write) if DEV_FEATURE is not enabled

@pcrespov This is about showing it, no updating.

pcrespov
pcrespov approved these changes Aug 26, 2025
View reviewed changes
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

thx

@odeimaiz odeimaiz merged commit 656c83d into ITISFoundation:master Aug 26, 2025
58 checks passed
@odeimaiz odeimaiz changed the title ✨ [Frontend] Allow setting phone number ✨ [Frontend] Expose phone number Aug 26, 2025
@GitHK GitHK mentioned this pull request Aug 26, 2025
Closed
14 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Sep 2, 2025
Closed
61 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Sep 18, 2025
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giancarloromeo giancarloromeo giancarloromeo left review comments

Copilot code review Copilot Copilot left review comments

@GitHK GitHK GitHK approved these changes

@pcrespov pcrespov pcrespov approved these changes

@sanderegg sanderegg Awaiting requested review from sanderegg

@matusdrobuliak66 matusdrobuliak66 Awaiting requested review from matusdrobuliak66

@bisgaard-itis bisgaard-itis Awaiting requested review from bisgaard-itis

Assignees

@odeimaiz odeimaiz

Labels

a:frontend issue affecting the front-end (area group) bug buggy, it does not work as expected t:enhancement Improvement or request on an existing feature

Projects

None yet

Milestone

Voyager

Development

Successfully merging this pull request may close these issues.

4 participants

@odeimaiz @pcrespov @giancarloromeo @GitHK