Skip to content

Conversation

@agamv25
Copy link

@agamv25 agamv25 commented Apr 30, 2025

Description

For this task, I first looked at different unit websites and the information provided by them, I first looked at the most commonly checked details, such as the credit points that the units have, the prerequisite units the unit must have and what units are required alongside the unit, so I first made changes to the unit database and added the creditpoint, prerequisite and corequisite units in the database, so previously where the courseflow would only show the unit name and ID, now it shows the creditpoints, prerequisite and the corequisite units in the code.

In the create unit form, now a new input form of credit points has also been added, which asks for the credit point from the admin when creating a new unit, in the database, the default values of the new unit for corequisite and the prerequisite are nil.

Fixes # (issue)

Type of change

image
image
image

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

How Has This Been Tested?

To test this unit, I logged in as the admin and created a new unit and checked if the code is made with a credit point or not, I then went to courseflow and then enterred the unit to check if the unit is being presented as desired.
When you place the unit in the study period and click on the "..." on the bottom of the unit cards to see the unit overlay with all the information for the unit
Make sure that you are on new/course-flow branch on the doubtfire-web and 9.x branch on doubtfire-api and doubtfire-deploy

Testing Checklist:

  • [ ✔] Tested in latest Chrome
  • Tested in latest Safari
  • Tested in latest Firefox

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 in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • [ ✔] My changes generate no new warnings
  • I have requested a review from @macite and @jakerenzella on the Pull Request

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.

Hi. I have fetch your PR and tested out the updated card components and it works good enough for a front-end mock-up. My advice is that instead of the attribute prerequisite and corequisite, you should replace that with level and and credit points since not all units have pre/corequisite. Additionally, you could take a look at my PR (#309) which I have added CSS so that the cards look similar to the courseflow mockup (https://ontrackdocumentation.netlify.app/courseflow/diagrams/) and apply that if you want.

@LovleenKala
Copy link

Hi Agam!
I was able to pull your request your onto my local machine. I have tested it on my Mac OS. I agree with Duy's comments and I am happy to give my review again once you have make the changes recommended by him.

@agamv25 agamv25 requested a review from Duy-Nguyen1104 May 6, 2025 09:07
@LovleenKala
Copy link

Hi Agam!
I was able to pull your request onto my local machine.
I tested the new Credit Points input field in the Create Unit Form, and it successfully saves the data to the database with proper validation.
The form now captures this extra data without breaking existing functionality.
Good Work!
Screenshot 2025-05-07 at 2 08 31 PM

@LovleenKala
Copy link

Hey Agam! I have reviewed your newly updated changes. Everything works well with the new updates.
Good Work.

Comment on lines +76 to +77
params.set('Unit Tutor first name', unitTutorFirstName);
params.set('Unit Tutor last name', unitTutorLastName);
Copy link

@aNebula aNebula Jun 7, 2025

Choose a reason for hiding this comment

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

These shouldn't be in courseflow - there is no need to include this, unless they are used.

startDate: Date;
endDate: Date;
selectedTeachingPeriod: number = null;
corequisite: string = "null";
Copy link

Choose a reason for hiding this comment

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

This should not be a "null" string, but just null.

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.

5 participants