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] Add seed and verification scripts for feedback chain #13074

Merged
merged 9 commits into from
Apr 22, 2024
Merged

[#12048] Add seed and verification scripts for feedback chain #13074

merged 9 commits into from
Apr 22, 2024

Conversation

marquestye
Copy link
Contributor

Part of #12048

Outline of Solution

  • Added methods to seed and verify feedback chain entities
  • Constraints - only seeds:
    • FeedbackTextQuestions, with
    • giverType = STUDENTS, and
    • recipientType = INSTRUCTORS
  • Tested for MAX_RESPONSE_COUNT values of -1 and 1000

@marquestye marquestye added the s.ToReview The PR is waiting for review(s) label Apr 21, 2024
@marquestye marquestye self-assigned this Apr 21, 2024
NicolasCwy
NicolasCwy reviewed Apr 22, 2024
View reviewed changes
Copy link
Contributor

@NicolasCwy NicolasCwy left a comment

Choose a reason for hiding this comment

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

Nice work on this PR! Could you add some count checks to VerifyCourseEntityAttributes. Left some comments which I'm curious about

src/client/java/teammates/client/scripts/sql/DataMigrationForCourseEntitySql.java Outdated Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/SeedDb.java Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/SeedDb.java
feedbackQuestion.setCreatedAt(getRandomInstant());
feedbackQuestion.setLastUpdate(getRandomInstant());

ofy().save().entities(feedbackQuestion).now();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the seed take long? Else we could try to persist all the questions at once

src/client/java/teammates/client/scripts/sql/VerifyCourseEntityAttributes.java Outdated Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/VerifyCourseEntityAttributes.java Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/VerifyCourseEntityAttributes.java Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/VerifyCourseEntityAttributes.java Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/VerifyCourseEntityAttributes.java Show resolved Hide resolved
src/client/java/teammates/client/scripts/sql/VerifyCourseEntityAttributes.java Show resolved Hide resolved
FergusMok
FergusMok approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

NicolasCwy
NicolasCwy approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@NicolasCwy NicolasCwy left a comment

Choose a reason for hiding this comment

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

LGTM!

@NicolasCwy NicolasCwy merged commit 25a6c91 into TEAMMATES:v9-course-migration Apr 22, 2024
1 check passed
@NicolasCwy NicolasCwy added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Task Other non-user-facing works, e.g. refactoring, adding tests and removed s.ToReview The PR is waiting for review(s) labels Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuanxi1 yuanxi1 yuanxi1 left review comments

@FergusMok FergusMok FergusMok approved these changes

@NicolasCwy NicolasCwy NicolasCwy approved these changes

@mingyuanc mingyuanc Awaiting requested review from mingyuanc

Assignees

@marquestye marquestye

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@marquestye @FergusMok @NicolasCwy @yuanxi1