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.

Sign up for GitHub

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

Jump to bottom

Fix #13798 remove warning check flag and now always warn leaving #20151

Merged
merged 10 commits into from
Apr 25, 2024
Merged

Fix #13798 remove warning check flag and now always warn leaving #20151

merged 10 commits into from
Apr 25, 2024

Conversation

Yushu-He
Copy link
Contributor

@Yushu-He Yushu-He commented Apr 11, 2024 •
edited
Loading

Overview

  1. This PR fixes Warning should appear if user accidently clicks the Back button in the lesson player. #13798 .
  2. This PR does the following: remove the check flag (isLoggedIn and isLoggedOutProgressTracked) for warn leaving and now always warn.

Essential Checklist

Please follow the instructions for making a code change.

  • I have linked the issue that this PR fixes in the "Development" section of the sidebar.
  • I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
  • I have written tests for my code.
  • The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
  • I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

unlogin.user.mp4
login.user.mp4

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

@Yushu-He Yushu-He requested a review from a team as a code owner April 11, 2024 15:38
@oppiabot OppiaBot
Copy link

oppiabot bot commented Apr 11, 2024

Assigning @Lawful2002 for the first pass review of this PR. Thanks!

@Yushu-He Yushu-He changed the title FIX #13798: remove warning check flag and now always warn leaving FIX #13798remove warning check flag and now always warn leaving Apr 11, 2024
@Yushu-He Yushu-He changed the title FIX #13798remove warning check flag and now always warn leaving FIX #13798 remove warning check flag and now always warn leaving Apr 11, 2024
@Yushu-He Yushu-He changed the title FIX #13798 remove warning check flag and now always warn leaving FIX issue #13798 remove warning check flag and now always warn leaving Apr 11, 2024
@Yushu-He Yushu-He changed the title FIX issue #13798 remove warning check flag and now always warn leaving Fix #13798 remove warning check flag and now always warn leaving Apr 11, 2024
Lawful2002
Lawful2002 approved these changes Apr 13, 2024
View reviewed changes
Copy link
Contributor

@Lawful2002 Lawful2002 left a comment

Choose a reason for hiding this comment

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

Thanks @Yushu-He, LGTM.
PTAL at the failing test.

@oppiabot OppiaBot
Copy link

oppiabot bot commented Apr 13, 2024

Unassigning @Lawful2002 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Apr 13, 2024
@oppiabot OppiaBot
Copy link

oppiabot bot commented Apr 13, 2024

Hi @Yushu-He, 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!

@seanlip
Copy link
Member

seanlip commented Apr 17, 2024

@Yushu-He Just to check, are you still planning to look at this? (See @Lawful2002's comment about the failing test.)

@Yushu-He
Copy link
Contributor Author

Yushu-He commented Apr 18, 2024 •
edited
Loading

@Yushu-He Just to check, are you still planning to look at this? (See @Lawful2002's comment about the failing test.)

@seanlip Yes, I was planning to check on this. But I am not very familiar with the e2e test and didn't get an idea why it fails as this should only affect when leaving the page. I doubt that maybe in the test some actions involve leaving the page and should be modified to adapt the new change.

@seanlip
Copy link
Member

seanlip commented Apr 19, 2024

@Yushu-He See lines 261-263 in core/tests/webdriverio/learnerFlow.js. I suspect the error lies around there.

Our wiki pages have info on the e2e tests if you want to understand how they work in more detail.

auto-merge was automatically disabled April 22, 2024 16:03

Head branch was pushed to by a user without write access

@Yushu-He Yushu-He requested a review from a team as a code owner April 22, 2024 16:03
@Yushu-He
Copy link
Contributor Author

@Yushu-He See lines 261-263 in core/tests/webdriverio/learnerFlow.js. I suspect the error lies around there.

Our wiki pages have info on the e2e tests if you want to understand how they work in more detail.

@seanlip Thanks for your advice! I now add acceptAlert to getHomePageWithAlert
.

But I don't know whether it is a good practice to do so. (Because I add a new method to libraryPage)

@oppiabot oppiabot bot unassigned Yushu-He Apr 22, 2024
@oppiabot OppiaBot
Copy link

oppiabot bot commented Apr 22, 2024

Unassigning @Yushu-He since a re-review was requested. @Yushu-He, please make sure you have addressed all review comments. Thanks!

seanlip
seanlip requested changes Apr 24, 2024
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks @Yushu-He, one comment.

core/tests/webdriverio_utils/LibraryPage.js Outdated
@@ -115,6 116,16 @@ var LibraryPage = function () {
);
};

this.getHomePageWithAlert = async function () {
await action.click('Oppia logo', oppiaLogo);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the way to do it -- the actions in this file should be actions taken by the user, and the user is not in control of whether they'll get an alert or not.

Instead, keep getHomePage() as it was and pass in a new boolean arg expectAlertToAppear. Add a comment to that method explaining in which circumstances the alert can appear. Update all existing references to getHomePage() to pass in false for that boolean, and pass in true at the point you need it in your test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I tried to revise the code and now it looks better.

@seanlip seanlip assigned Yushu-He and DubeySandeep and unassigned seanlip Apr 24, 2024
@seanlip
Copy link
Member

seanlip commented Apr 24, 2024

Also @DubeySandeep PTAL as codeowner, thanks.

@oppiabot oppiabot bot removed the PR: LGTM label Apr 24, 2024
@oppiabot OppiaBot
Copy link

oppiabot bot commented Apr 24, 2024

Hi, @DubeySandeep, the LGTM Label has been removed because the changes were requested on this PR. Thanks!.

@Yushu-He
Copy link
Contributor Author

@seanlip @DubeySandeep Please review the revised code, I think it is now good and pass the test.

seanlip
seanlip approved these changes Apr 25, 2024
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @Yushu-He, and congrats on your first PR to Oppia! 🎉

@seanlip seanlip added this pull request to the merge queue Apr 25, 2024
Merged via the queue into oppia:develop with commit 03fc029 Apr 25, 2024
80 checks passed
@Yushu-He Yushu-He deleted the warn-leaving branch April 25, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanlip seanlip seanlip approved these changes

@Lawful2002 Lawful2002 Lawful2002 approved these changes

@DubeySandeep DubeySandeep Awaiting requested review from DubeySandeep DubeySandeep is a code owner automatically assigned from oppia/dev-workflow-reviewers

Assignees

@DubeySandeep DubeySandeep

@Yushu-He Yushu-He

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warning should appear if user accidently clicks the Back button in the lesson player.
4 participants
@Yushu-He @seanlip @Lawful2002 @DubeySandeep