Skip to content

Conversation

@Duy-Nguyen1104
Copy link

Description

This pull request introduces foundational changes to the Unit model to support the upcoming Course Flow feature. It adds credit_points, prerequisites, and corequisites columns to the units table.

The primary motivation is to store and make readily available key unit attributes that will be displayed in an overlay component within the Course Flow UI. By denormalizing this data onto the Unit model, we can provide a faster and more responsive user experience, as it avoids the need for complex real-time queries to gather this information for display purposes.

The UnitsApi has also been updated to accept and persist these new attributes.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Logging the units inside the browser ahs shown that API response has included these attributes to the Unit entity:
image

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@Duy-Nguyen1104 Duy-Nguyen1104 force-pushed the feat/update-unit-model branch from 4ee087b to fe682de Compare August 3, 2025 12:45
@returnMarcco
Copy link

returnMarcco commented Aug 5, 2025

Hi @Duy-Nguyen1104,

This is my first backend review, so I'll do my best to review and test your changes.

Important: After a discussion with @Duy-Nguyen1104, I can confirm that there are ccmmits in this PR that are not relevant to the updating of the Unit model. This means that this review applies to the following commit only:
fe682de

Findings

  • I can see that three new fields have been added to the Units table - prerequisite, corequisite, and credit_points.
  • I can see that the unit API has been adjusted to accept these new fields.
  • The new migrations have been executed successfully with the Units table successfully updated with the new columns.

API Response Testing

  • I have made a GET request to the /api/units/ endpoint.
  • The response contains the following fields:
Screenshot 2025-08-06 092504
  • I can clearly see that the new fields have been returned in the response from the endpoint.

Suggestions

  • As mentioned, this review and approval applies only to the commit linked above. I suggest ensuring that the other commits contained in this PR are safe to be merged in at the same time.

All in all, well done.

Copy link

@returnMarcco returnMarcco left a comment

Choose a reason for hiding this comment

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

This approval only applies to commit hash fe682de46077e12aa4f7e033c9134aeefa1560ae.
Please ensure the other commits contained in this PR are safe to be merged upstream at the same time.

Copy link

@giangnht19 giangnht19 left a comment

Choose a reason for hiding this comment

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

The /api/units endpoint returns the expected response with proper structure and relevant data fields. I verified that:

  • Prerequisite, corequisite, and credit_points are already added to Unit model
  • The api has also been updated
image

Good work!

Copy link

@BrianDangDev BrianDangDev left a comment

Choose a reason for hiding this comment

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

  • Added API and model for managing course/unit requirements.
  • Units now support credit points, prerequisites, and corequisites.
  • If userId is nil in the course map API, it returns the template course map

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants