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

[DT-688] Disable DUOS sign-in when Sam is down #2763

Merged
merged 5 commits into from
Jan 10, 2025

Conversation

fboulnois
Copy link
Contributor

@fboulnois fboulnois commented Jan 7, 2025

Addresses

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

Summary

Takes the UI design from: https://github.com/DataBiosphere/duos-ui/tree/cc-dt-688-disable-ui-when-sam-down

  • Add Sam as a separate item in the status page to see at a glance
  • Disable DUOS sign-in when Sam is down
Screenshot 2025-01-09 at 11 16 40 AM

Have you read Terra's Contributing Guide 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

@fboulnois fboulnois force-pushed the fb-dt-688-check-sam-down branch from 9873d6b to 4b1b8a1 Compare January 8, 2025 01:23
beforeEach(() => {
cy.initApplicationConfig();
cy.stub(ServiceStatus, 'getConsentStatus').resolves(consentStatus);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason if I add this to the tests instead, the tests fail. This is very mysterious cypress behavior to me.

@fboulnois fboulnois changed the title Disable DUOS sign-in when Sam is down [DT-688] Disable DUOS sign-in when Sam is down Jan 8, 2025
@fboulnois fboulnois marked this pull request as ready for review January 8, 2025 13:42
@fboulnois fboulnois requested a review from a team as a code owner January 8, 2025 13:42
@fboulnois fboulnois requested review from pshapiro4broad and rjohanek and removed request for a team January 8, 2025 13:42
Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

👍🏽

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.

Do you have a screenshot to show what it looks like?

Comment on lines +180 to +182
const init = async () => {
setIsSamDown(!(await ServiceStatus.isSamHealthy()));
};
Copy link
Contributor

@rjohanek rjohanek Jan 8, 2025

Choose a reason for hiding this comment

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

why use a useEffect with no dependencies, instead of a onMount?

Copy link
Contributor Author

@fboulnois fboulnois Jan 8, 2025

Choose a reason for hiding this comment

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

That is the recommended mechanism when using modern React, it's equivalent to an onMount: https://react.dev/learn/synchronizing-with-effects#step-3-add-cleanup-if-needed


class Status extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you've refactored this component a bit; are we moving away towards extending Conponent?/Is this a step towards typescriptifying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and yes, this is the old React style, we have moved to a more functional style (export const Thing = (props) => { ... return (...) };).

@fboulnois
Copy link
Contributor Author

fboulnois commented Jan 8, 2025

Do you have a screenshot to show what it looks like?

See Summary section above.

@rjohanek
Copy link
Contributor

rjohanek commented Jan 8, 2025

Do you have a screenshot to show what it looks like?

See Summary section above.

Thanks! Is it useful to say "Sam"? Will that mean anything to users? Are there mocks for this?

@fboulnois
Copy link
Contributor Author

Do you have a screenshot to show what it looks like?

See Summary section above.

Thanks! Is it useful to say "Sam"? Will that mean anything to users? Are there mocks for this?

I don't think we have mocks and I'm not sure about the messaging, I reused Cinye's design for the UI aspect.

@rjohanek
Copy link
Contributor

rjohanek commented Jan 8, 2025

I don't think we have mocks and I'm not sure about the messaging, I reused Cinye's design for the UI aspect.

Maybe we could ask Lou, or use more broad language like "authentication"

@fboulnois fboulnois requested review from rjohanek and rushtong January 9, 2025 16:20
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.

looks good, thanks!

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

Love the new message 👍🏽

@fboulnois fboulnois merged commit c23831a into develop Jan 10, 2025
9 checks passed
@fboulnois fboulnois deleted the fb-dt-688-check-sam-down branch January 10, 2025 15:11
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.

3 participants