Test/repo privatisation #1342
Test/repo privatisation #1342
Conversation
| .next() | ||
| .find("input") | ||
| .as("passwordInput") | ||
| cy.wait(1000) |
There was a problem hiding this comment.
I was getting wonky results when attempting to inspect the contents of the password field immediately for some reason - I think it might have been because of the auto generation + population of the field taking some time. I've opted to add in a wait time here as this made the test less flaky, but open to suggestions on making this better!
There was a problem hiding this comment.
i can work with you on this - this shouldn't block the PR cos it works fine as-is but it is an issue because static wait times, if propagated, either waits for no reason (when wait time is insufficient) or causes extra delay (resolve early prior to wait).
the trick to solve this is to know what signals completion and either assert on that or wait on the request
| .parent() | ||
| .find("input") | ||
| .as("privatiseStaging") | ||
| cy.get("@privatiseStaging").should("be.checked") |
There was a problem hiding this comment.
this has implicit dep on prev test so if the prev one fail, this will fail too ._.
| .find("input") | ||
| .as("passwordInput") | ||
| cy.wait(1000) | ||
| cy.get("@passwordInput").should("have.length.gt", 0) |
There was a problem hiding this comment.
we should aim to make some simple assertions about the nature of this pw - eg, that it is random (might be hard) or that it is regenerated per toggle (easy + impt).
given that this is regarding security, i'm of the mind to make our invariants super clear
| let passwordSettings | ||
| const isLaunchDarklyImplemented = false | ||
| if (shouldGetPrivacyDetails && isLaunchDarklyImplemented) { | ||
| // TODO: LaunchDarkly to allow specific groups to access this feature first | ||
| passwordSettings = await SettingsService.getPassword({ siteName }) | ||
| } else { | ||
| passwordSettings = { | ||
| isAmplifySite: false, | ||
| password: "", | ||
| } |
There was a problem hiding this comment.
why's this here ._. also, we should remove the toggle as a whole rather than simply changing the request (+ enforce on BE also)
we should also use an env var for this because having it in code requires a full deploy, which defeats the purpose
There was a problem hiding this comment.
Accidental merge into the wrong branch - will put this in the main branch!
src/layouts/Settings/Settings.tsx
Outdated
| @@ -34,11 +36,13 @@ import { SocialMediaSettings } from "./SocialMediaSettings" | |||
|
|
|||
| export const Settings = (): JSX.Element => { | |||
| const { siteName } = useParams<{ siteName: string }>() | |||
| const { userId } = useLoginContext() | |||
| const isGithubUser = userId !== "Unknown user" && !!userId | |||
There was a problem hiding this comment.
could we add a note explaining hwy this means that it is a github user?
alexanderleegs
force-pushed
the
feat/repo-privatisation
branch
from
7cbfee3 to
10866c2
Compare
alexanderleegs
force-pushed
the
test/repo-privatisation
branch
from
b43612b to
54c6f7b
Compare
|
@alexanderleegs is this ready to be merged? |
Will need to address chin's comments first - but currently merging these tests are not particularly useful, because they always result in failure since the privatisation feature is hidden! |
|
This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers |
1 similar comment
|
This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers |
|
Tests are erroring because of the settings being a conditional field for email login repos only - going to close this for now, we can reference later on when creating tests for playwright for settings Edit: merging it in but skipping privatisation related tests instead |
alexanderleegs
force-pushed
the
test/repo-privatisation
branch
from
54c6f7b to
8ae1775
Compare
This PR adds tests for privatisation into our existing settings cypress tests. To facilitate this, the repo being used for the settings tests has been changed to the email login version, and the helper methods have been changed to fit the new changes.
Resolves IS-278