Skip to content

Conversation

@cinyecai
Copy link
Contributor

Addresses

https://broadworkbench.atlassian.net/browse/DT-697

Summary

If OAUTH2_CLAIM_name=unknown default the display name to the auth claim's email instead.


Have you read CONTRIBUTING.md lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@cinyecai cinyecai requested a review from a team as a code owner October 24, 2024 19:26
@cinyecai cinyecai requested review from fboulnois and snf2ye and removed request for a team October 24, 2024 19:26
Copy link
Contributor

@s-rubenstein s-rubenstein left a comment

Choose a reason for hiding this comment

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

Edit: I missed what Repo this was in - please ignore me

Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

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

Looks good to me. Are the unit tests for this class? If so, could this be tested there?

@cinyecai
Copy link
Contributor Author

Could you make it name === "unknown" rather than name.equals("unknown")? I know we have the null check before, but in case it is undefined I want to make sure we are using the safer check.

Can you explain a bit more about the distinction, just for my understanding? I don't think "===" can be used in Java.

@s-rubenstein
Copy link
Contributor

I misunderstood where this PR is - ignore me :|

@cinyecai
Copy link
Contributor Author

Looks good to me. Are the unit tests for this class? If so, could this be tested there?

Yes, there are tests for this class, I made sure they all still pass. I can add some testing to test this change too!

Copy link
Contributor

@rjohanek rjohanek left a comment

Choose a reason for hiding this comment

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

with an additional unit test, I think this looks good!

@cinyecai cinyecai merged commit 1b01113 into develop Oct 24, 2024
@cinyecai cinyecai deleted the cc-dt-697-unknown-display-name-bug branch October 24, 2024 20:49
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.

5 participants