Skip to content

Conversation

@agamv25
Copy link

@agamv25 agamv25 commented Apr 30, 2025

Description

Made changes in the database by migrating creditpoint, prerequisite and corequisite to the database to call it from the backend and use it in the front-end. Made changes to the units_api.rb, database_populator.rb, unit_entity.rb and schema.rb files
Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
    First I used Postman to get the unit details and to see if the changes are visible in the output
    image

  • Test B
    In the front-end I used the create unit file to create a new unit and check if the default values are present.

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
  • I have made corresponding changes to the documentation if appropriate
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have created or extended unit tests to address my new additions
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

If you have any questions, please contact @macite or @jakerenzella.

@LovleenKala
Copy link

Hi Agam!
I have pulled both the frontend and backend requests onto my local machine.
I was also able to test the backend using postman

Copy link

@Duy-Nguyen1104 Duy-Nguyen1104 left a comment

Choose a reason for hiding this comment

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

I tested it and the corresponding attribute shows. But for the already populated data, all the attributes are showing null:
"creditpoint": null,
"prerequisite": null,
"corequisite": null,
Will we have to update all the units available on Ontrack if this is merged into production? If this is no problem, then I will approve the PR

@agamv25
Copy link
Author

agamv25 commented May 8, 2025

I tested it and the corresponding attribute shows. But for the already populated data, all the attributes are showing null: "creditpoint": null, "prerequisite": null, "corequisite": null, Will we have to update all the units available on Ontrack if this is merged into production? If this is no problem, then I will approve the PR

I saw your code review and the default value for these rows I added for prerequisite and corequisite were null, as many units don't have any code values, so it makes more sense to populate them manually. If the front-end finds any value empty, it will show "null" on the front-end, so it shouldn't affect any functionality

Copy link

@b0ink b0ink left a comment

Choose a reason for hiding this comment

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

Hi @agamv25

Just a few notes regarding your prerequisite and corequisite columns that may need further consideration on how they'll be implemented.

Are your changes based off any documentation that covers the schema design approach?

expose :learning_outcomes, using: LearningOutcomeEntity, as: :ilos, unless: :summary_only
expose :tutorial_streams, using: TutorialStreamEntity, unless: :summary_only

# Expose staff before tutorials, so that their details are available
Copy link

Choose a reason for hiding this comment

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

This comment should probably remain in the code

Comment on lines +79 to +80
optional :prerequisite, type: String
optional :corequisite, type: String
Copy link

Choose a reason for hiding this comment

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

prerequisites and corequisites should probably be some sort of (array of) references to the existing unit definitions, instead of string

Comment on lines +3 to +4
change_column_default :units, :corequisite, from: nil, to: "Nil"
change_column_default :units, :prerequisite, from: nil, to: "Nil"
Copy link

Choose a reason for hiding this comment

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

corequisite and prerequisite should be of NULL type if empty

@agamv25
Copy link
Author

agamv25 commented May 11, 2025

Hi @agamv25

Just a few notes regarding your prerequisite and corequisite columns that may need further consideration on how they'll be implemented.

Are your changes based off any documentation that covers the schema design approach?

Yes, I made the changes according to the schema design approach from the Courseflow documentation

@b0ink
Copy link

b0ink commented May 11, 2025

Yes, I made the changes according to the schema design approach from the Courseflow documentation

Assuming you're referring to this documentation: https://thoth-tech.netlify.app/products/courseflow/documentation/design/backend-functional-requirements/#requirements, there is mention of a requirements entity.

The requirement entity contains a category field which determines if its a prerequisite or a corequisite. These two columns need to be removed and you will need to check if the requirement entity has been implemented yet, and check if the unit_entity contains this requirement reference.

Also, as you are working on the courseflow feature, please update your base branch from 9.x to the new/courseflow branch

@aNebula
Copy link

aNebula commented Jun 7, 2025

@AgamSingh4756 This PR has not passed senior leader review, and hence, is not ready and won't count as a complete task.

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.

6 participants