Skip to content

[IMPROVE] Inconsistent and misleading 2FA settings#22042

Merged
sampaiodiego merged 13 commits intodevelopfrom
2fa-settings-update
May 21, 2021
Merged

[IMPROVE] Inconsistent and misleading 2FA settings#22042
sampaiodiego merged 13 commits intodevelopfrom
2fa-settings-update

Conversation

@lucassartor
Copy link
Contributor

Proposed changes (including videos or screenshots)

Currently, there are some inconsistencies and incorrect behaviors on the 2FA settings, such as:

  • When disabling the TOTP 2FA, all 2FA are disabled;
  • There are no option to disable only the TOTP 2FA;
  • If 2FA are disabled, the other settings aren't blocked (the e-mail 2FA setting, for example);
  • It lacks some labels to warn the user of some specific 2FA options.

This PR looks to fix those issues.

Issue(s)

ClickUp Task

Steps to test or reproduce

All inconsistent behaviors can be reproduce in: Administration > Accounts > Two Factor Authentication.

Further comments

@lucassartor lucassartor requested a review from a team May 14, 2021 14:08
KevLehman
KevLehman previously approved these changes May 14, 2021
@KevLehman KevLehman requested a review from a team May 17, 2021 16:03
KevLehman
KevLehman previously approved these changes May 20, 2021
@sampaiodiego
Copy link
Member

looks like tests are not passing due to something else, not sure why it broke only now though 🤔

@lucassartor can you pls fix the tests?

@pierre-lehnen-rc
Copy link
Contributor

The error seems related to the new message parser

@sampaiodiego
Copy link
Member

The error seems related to the new message parser

right.. I just saw there was a describe.only that was removed, which may explain why the error showed up only now 😬

@sampaiodiego
Copy link
Member

sampaiodiego commented May 20, 2021

so whoever added the describe.only in the first place should fix the tests now 😂 jk 🙈

@lucassartor
Copy link
Contributor Author

There's actually a enableMessageParserEarlyAdoption missing on the /me test in miscellaneous. As Pierre mentioned the new message parser was implemented but someone forgot to update this specific test (probably added by the merge branch with develop that Pierre did). So, as I removed the describe.only, this error is now showing. I'll fix it tho.

@lucassartor lucassartor dismissed stale reviews from pierre-lehnen-rc and KevLehman via 3a62755 May 20, 2021 20:04
@lucassartor
Copy link
Contributor Author

Another error is showing now. Apparently is not related to this PR though. I'll check it out.

@sampaiodiego sampaiodiego merged commit d3acf8b into develop May 21, 2021
@sampaiodiego sampaiodiego deleted the 2fa-settings-update branch May 21, 2021 02:12
@sampaiodiego sampaiodiego mentioned this pull request May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments