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

[#12048] Migrate Tests for FeedbackSessionPublishedRemindersActionTest #13223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[#12048] Migrate Tests for FeedbackSessionPublishedRemindersActionTest #13223

wants to merge 1 commit into from
+135 −0

Conversation

DhiraPT
Copy link
Contributor

@DhiraPT DhiraPT commented Feb 9, 2025

Part of #12048

Outline of Solution
Migrated FeedbackSessionPublishedRemindersActionTest to use SQL-based logic instead of the previous Datastore model.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 9, 2025

Hi @DhiraPT, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

xenosf
xenosf approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@xenosf xenosf 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 for your hard work :)

xenosf
xenosf requested changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@xenosf xenosf left a comment

Choose a reason for hiding this comment

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

Apologies – I missed one small thing in my review for this. Looks good apart from that!

src/test/java/teammates/sqlui/webapi/FeedbackSessionPublishedRemindersActionTest.java
logoutUser();
verifyCannotAccess();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the original test, testAccessControl() also tested that unregistered users cannot access it, but this case is not present in the migrated test.

You can use loginAsUnregistered(id) to log in as an unregistered user

jasonqiu212
jasonqiu212 approved these changes Feb 27, 2025
View reviewed changes
Copy link
Contributor

@jasonqiu212 jasonqiu212 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, apart from the above suggestion! good work

@jasonqiu212 jasonqiu212 added s.FinalReview The PR is ready for final review s.Ongoing The PR is being worked on by the author(s) and removed s.FinalReview The PR is ready for final review labels Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xenosf xenosf xenosf requested changes

@jasonqiu212 jasonqiu212 jasonqiu212 approved these changes

@dishenggg dishenggg Awaiting requested review from dishenggg

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
s.Ongoing The PR is being worked on by the author(s)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DhiraPT @jasonqiu212 @xenosf