Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add Playwright tests for OIDC-aware & OIDC-native #12252

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Feb 15, 2024

Stabilises raciness of the Manage account button in Settings also


This change is marked as an internal change (Task), so will not be included in the changelog.

@t3chguy t3chguy added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Feb 15, 2024
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I'm halfway through this review, and whilst I've seen a lot of docker magic I half understand, I've seen nothing to do with the purported purpose of this PR. Please could you break it up to make it more digestible?

src/stores/oidc/OidcClientStore.ts Show resolved Hide resolved
Comment on lines +176 to 180
const delegatedAuthAccountUrl = useAsyncMemo(async () => {
await sdkContext.oidcClientStore.readyPromise; // wait for the store to be ready
return sdkContext.oidcClientStore.accountManagementEndpoint;
}, [sdkContext.oidcClientStore]);
const disableMultipleSignout = !!delegatedAuthAccountUrl;
Copy link
Member

Choose a reason for hiding this comment

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

I haven't read through the whole class in detail, but won't setting delegatedAuthAccountUrl to undefined while we make a bunch of network requests mean that we display the wrong UI for a while? We need to distinguish between "we don't know yet" and "we're definitely not OIDC".

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, no design was given for this case so this is what we're going with for the time being.

playwright/plugins/postgres/index.ts Outdated Show resolved Hide resolved
playwright/plugins/postgres/index.ts Outdated Show resolved Hide resolved
playwright/plugins/docker/index.ts Outdated Show resolved Hide resolved
playwright/plugins/docker/index.ts Outdated Show resolved Hide resolved
playwright/plugins/docker/index.ts Outdated Show resolved Hide resolved
playwright/plugins/docker/index.ts Outdated Show resolved Hide resolved
playwright/plugins/docker/index.ts Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member Author

t3chguy commented Feb 19, 2024

I've seen nothing to do with the purported purpose of this PR

Yup as per your ask on a different PR I split it into sane commits; so the actual main bit of the PR is the last commit - e13048d - the rest are all atomic building blocks for the test to work upon

@richvdh
Copy link
Member

richvdh commented Feb 19, 2024

Yup as per your ask on a different PR I split it into sane commits; so the actual main bit of the PR is the last commit - e13048d - the rest are all atomic building blocks for the test to work upon

Well, separate commits are certainly helpful, but this is still an absolutely epic PR which is making a bunch of different changes. I really think it would be helpful to break it up into smaller PRs, to make the review cycle easier (as well as all the other usual reasons we use separate PRs rather than a single massive two-weekly PR). Please?

@t3chguy
Copy link
Member Author

t3chguy commented Feb 19, 2024

Split out #12260 & #12261

… OIDC mode

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy t3chguy added this pull request to the merge queue Feb 21, 2024
Merged via the queue into develop with commit 36a8d50 Feb 21, 2024
26 checks passed
@t3chguy t3chguy deleted the t3chguy/e2e-oidc-aware branch February 21, 2024 11:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants