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
Fix part of #17712 Add more acceptance tests for logged in users #20126
base: develop
Are you sure you want to change the base?
Conversation
…nto static-pages
…l likns on get started page spec
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.
Thanks @agallop ! I don't really have comments beyond what Christie has :)
*/ | ||
async clickLinkToHomePageOnPrivacyPolicyPage(): Promise<void> { | ||
await this.page.waitForXPath( | ||
'//a[contains(text(),"https://www.oppia.org")]' |
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.
Yes, I think so -- the policy is only for the oppia.org site.
this.clickOn('https://www.oppia.org'), | ||
]); | ||
|
||
expect(this.page.url()).toBe(learnerDashboardUrl); |
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.
I think it redirects to the learner dashboard if you are signed in ... are you sure you are signed in?
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.
I commented on the function clickLinkToHomePageOnPrivacyPolicyPage in logged-in-users-utils.ts
this.clickOn('https://www.oppia.org'), | ||
]); | ||
|
||
expect(this.page.url()).toBe(learnerDashboardUrl); |
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.
Yes, I'm sure I signed in and when I clicked the link. It navigates to 'https://www.oppia.org' instead of the learner dashboard.
The redirecting to the learner dashboard only happens if the link is localhost:8181.
@imchristie @seanlip Please take another look |
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.
LGTM!
Unassigning @imchristie since they have already approved the PR. |
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.
Sorry for the late reply -- LGTM for my original comments!
@agallop There's a test failing for the footer -- could you PTAL?
|
Hi @agallop, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
Here are the github action runs for the acceptance tests for this PR which shows that:
Action summary
Proof of changes on desktop with slow/throttled network
N/A
Proof of changes on mobile phone
Action summary
Proof of changes in Arabic language
N/A
PR Pointers