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.

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

[#12048] Migrate InstructorNotificationsPageE2E #12906

Merged
merged 9 commits into from
Mar 18, 2024

Conversation

yuanxi1
Copy link
Contributor

@yuanxi1 yuanxi1 commented Mar 16, 2024

Part of #12048

Outline of Solution

  1. Migrate InstructorNotificationsPageE2E test to use sql entities
  2. Encountered an error when loading the sql data bundle. In particular, Hibernate fails to persist Instructor entity as the Course entity it refers to is not in managed state by Hibernate (some info on entity state transition)
Screenshot 2024-03-16 at 3 28 48 PM

From this stackoverflow post, possible cause is the unidirectional @ManyToOne relation on User but no @OneToMany with CascadeType.ALL on the Course side.

Adding @OneToMany + CascadeType.ALL in Course can fix the error, however since one course usually has many users, this may lead to slower performance.

As mentioned in the same post, one fix without adding @OneToMany + CascadeType.ALL is to explicitly set the parent in child after parent is created, which is adopted as a temporary fix in this pr.

Copy link

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

  • Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]
  • 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.

@yuanxi1 yuanxi1 changed the title [12048] Migrate InstructorNotificationsPageE2E [#12048] Migrate InstructorNotificationsPageE2E Mar 16, 2024
@weiquu
Copy link
Contributor

weiquu commented Mar 18, 2024

Hi @yuanxi1, thanks for the research! I think the temp fix is fine - we'll probably try to resolve this when refactoring entities. Could you add the test to the .xml file so that it runs on github actions? Can take a look at the tele message for more details

edit: sorry pls ignore the first comment haha - refer to what u discussed with cedric

Copy link
Contributor

@cedricongjh cedricongjh 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 the work and investigating the issue with persisting the databundle!

@cedricongjh cedricongjh requested a review from weiquu March 18, 2024 08:47
@cedricongjh cedricongjh added the s.FinalReview The PR is ready for final review label Mar 18, 2024
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM!

@weiquu weiquu added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Mar 18, 2024
@cedricongjh cedricongjh merged commit a34c3c5 into TEAMMATES:master Mar 18, 2024
11 checks passed
@cedricongjh cedricongjh added this to the V9.0.0-beta.1 milestone Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants