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

Updating the Certification Page for course based trainings #2223

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

Rutvikrj26
Copy link
Contributor

@Rutvikrj26 Rutvikrj26 commented Apr 22, 2024

What

This is a small PR making a tiny change in the queryset for the trainings. The logic is updated to also include custom logic for courses as their expiry is determined in slightly different way than the regular one.

Why

A TrainingType can have multiple courses attached to it, each being a unique version in itself. Each course version should have it's own valid duration - and hence it's own expiry date.

Whenever a user takes an on-platform course, it is the latest version of the course available at that time, but with time new versions can come in, and at some point, the version of the course that this training was attached to, will be deprecated. At that point all the trainings associated with this course version should be renewed. Thus, admin panel provides an option to set an expiry date to the associated trainings when deprecating a course version. That is how the expiry date for the trainings is decided for course version, which is different from the way it is determined for other types of trainings.

Thus, this PR adds onto the logic of querysets so that all the courses are accurately identified and put under active / expired versions.

This PR does not disturb the regular flow - it only add onto the logic to also fetch the course based trainings (A feature in the upcoming Trainings part-2 PR)

@Rutvikrj26 Rutvikrj26 marked this pull request as ready for review April 24, 2024 19:49
@Rutvikrj26
Copy link
Contributor Author

The model connection between Course and Trainings has now been updated to a one-to-many relationship. The initial plan of implementing the M2M was to avoid adding new fields to training. For example, the training_type is attached to course via a foreign key in course, thereby not changing the training_type model, but was not the best implementation and has hence been changed to the more logical implementation - one training can only have one course, but a course can have multiple trainings.

That implementation requires a migration and a new table to be created, which is created in the #1951.
This PR should only be merged after merging #1951 else it will break the current Certifications Page

@Rutvikrj26 Rutvikrj26 changed the title Updating the query set logic to include the expiry check for course based trainings Updating the Certification Page for course based trainings Jul 24, 2024
@Rutvikrj26 Rutvikrj26 force-pushed the rs/updating_query_logic_for_training_status branch from 53a918c to 9a5febe Compare August 27, 2024 19:58
@Rutvikrj26
Copy link
Contributor Author

@tompollard @bemoody

When a user is taking a course, we only add a training in the system at the end of the course. The training directly gets to the Accepted stage. To show the courses in progress, I am thinking of making a training object at the start of the course and assign it the review status. When the user completes the course, that training object's status gets updated to Accepted.

Furthermore, I'm also thinking of changing the Under Review to In Progress in the certifications page. Please let me know your thoughts on this.

image

@Rutvikrj26
Copy link
Contributor Author

@tompollard @bemoody I've tried summarizing the larger goal & the smaller chunks the entire Course functionality was broken into #2299

This is the third part of the functionality required to display the in-progress, active & expired courses.

Copy link
Member

@tompollard tompollard left a comment

Choose a reason for hiding this comment

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

I added a couple of suggestions, but overall this looks okay to me. The mixed tutorial types is fairly confusing, and I hope we can clear this up at some point.

physionet-django/training/views.py Outdated Show resolved Hide resolved
physionet-django/user/managers.py Show resolved Hide resolved
physionet-django/user/managers.py Outdated Show resolved Hide resolved
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.

2 participants