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

USH-4497: Do not append domain to username with @ #1579

Merged
merged 1 commit into from
May 3, 2024

Conversation

MartinRiese
Copy link
Contributor

@MartinRiese MartinRiese commented May 2, 2024

Technical Summary

https://dimagi.atlassian.net/browse/USH-4497

Users now can login as web users which already have the complete username used for couch. Do not append domain and host it that case. Without this change the claim-case request has a non-existing username in the body and will fail silently.

We do the same in hq: https://github.com/dimagi/commcare-hq/pull/34351/files#diff-b5520648dffd5a4f57530e5e1c36f90cd022db2cbf59bfef9bb3d65997d6f316R64-R74

Safety Assurance

Safety story

Automated test coverage

QA Plan

Special deploy instructions

  • This PR can be deployed after merge with no further considerations.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations.

Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change.

Copy link

codecov bot commented May 2, 2024

Codecov Report

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

Project coverage is 70.39%. Comparing base (db2caad) to head (52427cf).
Report is 2 commits behind head on master.

Files Patch % Lines
...java/org/commcare/formplayer/util/StringUtils.java 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1579      +/-   ##
============================================
- Coverage     70.41%   70.39%   -0.02%     
  Complexity     2009     2009              
============================================
  Files           251      251              
  Lines          7797     7799       +2     
  Branches        711      711              
============================================
  Hits           5490     5490              
- Misses         2037     2038       +1     
- Partials        270      271       +1     

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

@MartinRiese
Copy link
Contributor Author

@shubham1g5 the function I changed is called twice. Once in the menu session, which is where the username for the claim case request comes from. And the second time in the RestoreFactory. I could not figure out when that code gets called and if the change would be appropriate there or if I should move the check.

@shubham1g5
Copy link
Contributor

And the second time in the RestoreFactory.

That usage looks harmless as the code that calls it is only getting used for logging without any affect on business logic.

@@ -9,6 +9,9 @@ public static String getFullUsername(String user, String domain, String host) {
}

public static String getFullUsername(String user, String domain) {
if (user.contains("@")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

since this method is only overloading the method before this, this change should actually be made in the previous method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean in MenuSession.getNextScreen correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

no there is another getFullUsername method that this method calls.

@shubham1g5
Copy link
Contributor

Q: Can't a Mobile worker name be like - user@1 on HQ ?

@MartinRiese
Copy link
Contributor Author

A: No they cannot. If you try using an @ during creation you get a validation error.

* Users now can login as web users which already have the complete
  username used for couch. Do not append domain and host it that
  case
@MartinRiese MartinRiese merged commit 4d149b6 into master May 3, 2024
4 of 6 checks passed
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.

None yet

2 participants