-
Notifications
You must be signed in to change notification settings - Fork 319
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
feat: replace app store button for ov enrollment #3671
feat: replace app store button for ov enrollment #3671
Conversation
@@ -1014,6 +1014,19 @@ test | |||
await t.expect(await enrollOktaVerifyPage.getSameDeviceSetupOnMobileText()).contains(sameDeviceOVEnrollmentInstructions3); | |||
|
|||
await t.expect(await enrollOktaVerifyPage.orAnotherMobileDeviceLinkExists()).eql(true); | |||
|
|||
if (!userVariables.gen3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like this fix will happen for gen3 later on? Do we have a ticket for that and could we potentially link it here as an inline comment + reminder so this work doesn't slip through the cracks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no mock has been provided for gen3, so change only apply to gen2 for now, aligned with @lesterchoi-okta
await t.expect(downloadInstruction).contains('Don’t have Okta Verify?'); | ||
await t.expect(downloadInstruction).contains('Download here'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you separate this into two different .contains()
calls because of the  
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort of, tried to assert eql against innerText of the element, not sure what's the cause, by testcafe assets two same string as false, then I followed pattern from
await t.expect(content).contains('Don’t have Okta Verify?'); |
const downloadInstruction = await enrollOktaVerifyPage.getSameDeviceDownloadText(); | ||
await t.expect(downloadInstruction).contains('Don’t have Okta Verify?'); | ||
await t.expect(downloadInstruction).contains('Download here'); | ||
await t.expect(await enrollOktaVerifyPage.appStoreElementExists()).eql(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion is fine for now to differentiate between gen2 and gen3 UX, but we should make sure to remove it once the app store logo does not exist for both generations since it doesn't feel necessary to assert that an image that we know DNE is no longer there
{{#if sameDeviceOVEnrollmentEnabled}} | ||
<li> | ||
<span>{{i18n code="customUri.required.content.download.title" bundle="login"}}</span> | ||
<a href="{{deviceMap.downloadHref}}" target="_blank" id="download-ov" class="orOnMobileLink"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but does the download-ov
ID serve any additional purpose besides being used as a selector in the page object? If not, could we just use the orOnMobileLink
class as the selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, just following existing pattern of how the link is constructed
Description:
UX change to replace app store button for ov enrolment
PR Checklist
Issue:
Reviewers:
Screenshot/Video:
Mobile


Desktop


Downstream Monolith Build: