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

create training basics --> Add create training from admin console by uploading json #1950

Conversation

amitupreti
Copy link
Contributor

@amitupreti amitupreti commented Mar 28, 2023

What?

Here i have added a feature that will allow admin to create training from admin console by uploading json file. This also lays foundation to implement take training feature and resume training feature for user later.

Why?

A training feature was desired by HDN so that we can build/customize a onplatform training for users which will ultimately be used to control access to projects.
The new onplatform training will work just like the existing Trainings and work with projects without any additional changes, Currently users take training outside the platform and then upload the certificate to the platform.

How?

We created a separate app called Training. This app will be used to create/manage training courses, and allow users to take onplatform training.

We created two major types of Models in training.models

1. Platform Training (training.models.OnPlatformTraining)

The idea here is that a new Training Course will be defined with user.models.TrainingType. This is the ultimate(top) model for a training course.

The training content for TrainingType model will be implemented by the new training app. On Training app, OnPlatformTraining instance can be created for each TrainingType. For different versions of the same training course, we can create as many OnPlatformTraining models as we want as long as the version is different.

A training is divided into modules. Each module has a description and a list of contents and quizzes. Modules are like chapters in a book. Each module has a list of contents and quizzes. Contents are like paragraphs in a chapter. Quizzes are like questions in a chapter. Contents and quizes can be organized in any order which is controlled by order field. For ordering the modules, contents and quizzes, we have used order field. The ordering is unique for each instance of the parent model.

For example : Under Module 1, we have content 1(order=1), content 2(order=2), content 3(order=4) and quiz 1(order=3). Then the ordering of the contents and quizzes will be content 1, content 2, quiz 1, content 3.

It is expected that the order will start from 1 and will be incremented by 1 with no gaps.

2. Tracking User Progress during training (training.models.OnPlatformTrainingProgress)

When a user starts a training, a OnPlatformTrainingProgress instance should be created for that user and the version of the training type that they are taking. This model tracks the progress of the user during the training. Similarly, when a user starts a module, a ModuleProgress model should be created for each module in the training. For quiz, content progress, we should create a instance of CompletedContent or CompletedQuiz model when the user completes a content or quiz.

I don't think we need to track when someone started a content or quiz as these are expected to complete in few minutes.

Testing?

Next PR #1951

Screenshots (optional)

Anything Else?

How to create a training from admin console?

  1. Login as admin
  2. Navigate to Training tab in admin console
  3. Click on Create/Update OP Training button
  4. Select Create New Training from the dropdown
  5. Upload the example json from training/fixtures/example-training-create.json

How to edit a training from admin console?

  1. Follow steps 1-3 from above
  2. Select Existing Training from the dropdown
  3. Upload the example json from training/fixtures/example-training-update.json

Notes:
This is the first part of Training PR, there is another PR #1951 which is rebased on this PR.

@amitupreti amitupreti marked this pull request as draft March 28, 2023 20:14
@amitupreti amitupreti force-pushed the au/training/feature/create_training_basics__create_training_admin branch from 1b3e9dd to 54f8325 Compare March 28, 2023 20:44
@amitupreti amitupreti marked this pull request as ready for review March 28, 2023 21:27
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've taken a quick look and added some comments. I didn't get all of the way through the changes, so will need to take another look later. It would be good to chat more about the approach, perhaps at our Tuesday meeting.

physionet-django/static/sample/create.json Outdated Show resolved Hide resolved
physionet-django/static/sample/create.json Outdated Show resolved Hide resolved
physionet-django/training/models.py Outdated Show resolved Hide resolved
physionet-django/training/models.py Outdated Show resolved Hide resolved
@alistairewj
Copy link
Member

Could we rename it from Training to Course for clarity/mental separation from other parts of the platform?

@amitupreti
Copy link
Contributor Author

Thank you @tompollard @alistairewj working on the feedback

@amitupreti amitupreti force-pushed the au/training/feature/create_training_basics__create_training_admin branch 2 times, most recently from 298170f to 657ac5c Compare April 10, 2023 16:06
Copy link
Member

@alistairewj alistairewj left a comment

Choose a reason for hiding this comment

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

Have some brief organizational comments. Haven't reviewed the models yet but I plan to!

physionet-django/training/models.py Outdated Show resolved Hide resolved
physionet-django/training/models.py Outdated Show resolved Hide resolved
physionet-django/training/models.py Outdated Show resolved Hide resolved
physionet-django/training/models.py Outdated Show resolved Hide resolved
physionet-django/notification/utility.py Outdated Show resolved Hide resolved
physionet-django/static/sample/create.json Outdated Show resolved Hide resolved
physionet-django/physionet/urls.py Show resolved Hide resolved
physionet-django/training/views.py Outdated Show resolved Hide resolved
@amitupreti amitupreti force-pushed the au/training/feature/create_training_basics__create_training_admin branch from 657ac5c to b304d1b Compare April 12, 2023 16:37
@amitupreti
Copy link
Contributor Author

Thanks @alistairewj , i addressed the comments.

Brief summary of updates,

  1. Move Course page inside console app
  2. Add an option to download the course data as json(which users can use to migrate the content or make some changes and update the course). Here i added options to allow admins to select the version of course they want to download also.
  3. Add a guideline page on how to create course

image

image

image

@amitupreti amitupreti force-pushed the au/training/feature/create_training_basics__create_training_admin branch from b304d1b to a369d8e Compare April 12, 2023 20:09
Copy link
Member

@alistairewj alistairewj left a comment

Choose a reason for hiding this comment

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

This is really great! I've made some comments on refactoring / rewriting some bits.

physionet-django/console/views.py Show resolved Hide resolved
physionet-django/training/views.py Show resolved Hide resolved
physionet-django/training/serializers.py Outdated Show resolved Hide resolved
physionet-django/training/serializers.py Show resolved Hide resolved
physionet-django/training/serializers.py Outdated Show resolved Hide resolved
physionet-django/training/models.py Outdated Show resolved Hide resolved
@amitupreti
Copy link
Contributor Author

This is really great! I've made some comments on refactoring / rewriting some bits.

Thanks @alistairewj , just saw this. Weirdly it was in my spam(Gmail thought there was some dangerous link in the email, 😆 )

image

@alistairewj
Copy link
Member

probably the links to physionet-build ;)

@tompollard
Copy link
Member

@Rutvikrj26 Try running makemigrations. I think tests are failing because the PR is missing a migration.

@tompollard
Copy link
Member

Tests just failing on a style issue in the migration now (E501 line too long):

physionet-django/training/migrations/0002_alter_course_unique_together_and_more.py:25:120: E501 line too long (124 > 119 characters)

             model_name='course',
             name='version',
             field=models.CharField(blank=True, default='', max_length=15, validators=[project.validators.validate_version]),
                                                                                                                        ^

physionet-django/training/models.py:23:17: E1[26](https://github.com/MIT-LCP/physionet-build/actions/runs/5731564942/job/15532734091?pr=1950#step:15:27) continuation line over-indented for hanging indent

         ]
         permissions = [
                 ('can_view_course_guidelines', 'Can view course guidelines'),
                 ^

physionet-django/training/models.py:24:13: E121 continuation line under-indented for hanging indent

         permissions = [
                 ('can_view_course_guidelines', 'Can view course guidelines'),
             ]
             ^

@Rutvikrj26 Rutvikrj26 force-pushed the au/training/feature/create_training_basics__create_training_admin branch from 67f0afa to 644c99c Compare January 9, 2024 12:42
@Rutvikrj26
Copy link
Contributor

  • The Create/Update button should be renamed Create and it should focus on creating new courses. Updating courses should happen on the page that lists all active/archived versions of a single course.

@tompollard This has been implemented and is ready for testing.
Furthermore, @bemoody it would be great if you could review the PR and let me know your thoughts.

@tompollard
Copy link
Member

tompollard commented Feb 6, 2024

I took a look this morning and have added a few thoughts below:

Minor formatting things:

  • The menu item could be cleaned up (capitalise first letter for consistency, use a unique icon)
  • Some files are missing newlines at the end.

Usability:

  • The page that displays Active/Archived courses is really not very clear from a user perspective. For example: What happens to a course when I set it to "expire"? The course is moved to "archived", but the expiry date disappears. How can I tell whether a course is expired or not?

If you haven't already, I'd suggest running through the design of this new feature with January. I think the page would benefit from some tweaks to layout and content. Perhaps also add some short explanatory sentences to the page to help guide the user?

TypeError when trying to update a course:

When attempting to update a course using the following JSON (!) (taken from the suggested example), I receive a TypeError:

{
  "name": "Course 2",
  "description": "<p>Test content description</div>",
  "valid_duration": "1095 00:00:00",
  "courses": [
    {
      "version": 2.0,
      "modules": [
        {
          "name": "Module 1",
          "description": "<p>Module description</div>",
          "order": 0,
          "contents": [
            {
              "body": "<p>Hello This is a test</p><div><p>Test content1</p></div>",
              "order": 0
            }
          ],
          "quizzes": [
            {
              "question": "What is the correct answer(choice1)?",
              "order": 1,
              "choices": [
                {
                  "body": "I am a choice1",
                  "is_correct": true
                },
                {
                  "body": "I am a choice2",
                  "is_correct": false
                },
                {
                  "body": "I am a choice3",
                  "is_correct": false
                },
                {
                  "body": "I am a choice4",
                  "is_correct": false
                }
              ]
            }
          ]
        }
      ]
    }
  ]
}
Request Method: POST
Request URL: http://localhost:8000/console/courses/3/

Django Version: 4.1.10
Python Version: 3.10.13
Installed Applications:

Traceback (most recent call last):
  File "/Users/tompollard/projects/physionet-build/env/lib/python3.10/site-packages/django/core/handlers/exception.py", line 56, in inner
    response = get_response(request)
  File "/Users/tompollard/projects/physionet-build/env/lib/python3.10/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/Users/tompollard/projects/physionet-build/env/lib/python3.10/site-packages/django/contrib/auth/decorators.py", line 23, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "/Users/tompollard/projects/physionet-build/env/lib/python3.10/site-packages/django/contrib/auth/decorators.py", line 23, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "/Users/tompollard/projects/physionet-build/physionet-django/training/views.py", line 104, in course_details
    if validate_version(new_course_version) is not None:
  File "/Users/tompollard/projects/physionet-build/physionet-django/project/validators.py", line 90, in validate_version
    if not re.fullmatch(r'[0-9]+(\.[0-9]+)+', value) or '..' in value:
  File "/opt/homebrew/Cellar/python@3.10/3.10.13/Frameworks/Python.framework/Versions/3.10/lib/python3.10/re.py", line 195, in fullmatch
    return _compile(pattern, flags).fullmatch(string)

Exception Type: TypeError at /console/courses/3/
Exception Value: expected string or bytes-like object

@bemoody
Copy link
Collaborator

bemoody commented Feb 6, 2024

Some things that jump out at me:

  • Courses should be identified by slug plus version (rather than integer ID plus version.) Either we want a slug field in the TrainingType, or in the Course.

  • I think the course metadata should be part of the versioned object. That is, the course title (what you've called name, I would call title) and description, and possibly also valid_duration.

  • It is very strange to have "courses" in the JSON file. If I understand correctly, one JSON file is one Course (one version.) So why is this an array?

  • The function of the is_active flag is not totally clear to me. Let's break down the expected life-cycle of a course:

  1. Version 1.0 is published. It's the only version. People requesting access are directed to take this course. People who have completed it are treated as "certified" for five years (or 100 years, or whatever) after the completion date.

  2. Version 1.1 is published. People requesting access are directed to take version 1.1. But people who already completed version 1.0 are still treated as "certified" for five years after completion.

  3. Version 2.0 is published. People requesting access are directed to take version 2.0. People who already completed version 1.0 are now treated as "certified" for at most 30 days after the date that version 2.0 was published.

How would these three states be represented in terms of the Course model?

(Note that update_course_for_version_change appears to be modifying the stored completion date in order to enforce what is actually an altered validity period. That seems like a bad idea to me.)

@Rutvikrj26
Copy link
Contributor

Please checkup the following workflow for new training addition and an old training expiry. Let me know if they sound okay.

Admin adds a new training-type & a user wants to get credentialed

  • User tries to take the course(belongs to a training-type) and is given the latest course for the training-type( to be ensured in course taking - second PR)
  • User completes the quizzes
  • User gets the certification / training attached to his account for the training type

Admin adds another course (a new version) for an existing training-type

  • The admin sees both the training type as active in the admin panel
  • Admin chooses to expire the previous version of the training
  • Admin goes into admin panel, puts in the number of days from now should any training/certification accredited for the "to-be-expired" version should be valid.
  • The validity duration reduces( the logic can be modified to accommodate a safe change - change in validity duration / adding an expiry_date field / somethingelse - any suggestions?)
  • The user gets notified to re-take the course for the training type.
  • user takes the new course and gets credentialed again.

@Rutvikrj26
Copy link
Contributor

When attempting to update a course using the following javascript (taken from the suggested example JSON), I receive a TypeError:

The type for version has changed to string to match the versioning of the projects. The schema examples have become outdated. Will be updating them in the next push.

@Rutvikrj26
Copy link
Contributor

Rutvikrj26 commented Feb 21, 2024

@tompollard @bemoody

As per our discussion, I have most of the changes implemented except two :

update_course_for_version_change appears to be modifying the stored completion date in order to enforce what is actually an altered validity period. That seems like a bad idea to me.

This is the only way as the trainings does not have any other field that can be updated to implement the change in validity.
Any suggestions - if we do want to gear away, we might need to add another field to the trainings model. Please let me know if there is some other way to implement this that I have missed.

  • Either we want a slug field in the TrainingType, or in the Course.

The current navigation uses trainingtype id and currently, TrainingType model does not have a slug field. Do we want to allow null for the slug value or is there a better strategy for back filling the slug that is preferred.

Please provide your thoughts on these two.

@Rutvikrj26
Copy link
Contributor

The current navigation uses trainingtype id and currently, TrainingType model does not have a slug field. Do we want to allow null for the slug value or is there a better strategy for back filling the slug that is preferred.

As discussed during the tuesday standup, a good idea will be use django-slugify for bacfilling the slugs in training type.
I've implemented the slug field in training type, added a migration to handle the backfilling and updated the routes & views to work with slug rather thank primary key.

@@ -92,6 +93,8 @@

# guidelines
path('guidelines/review/', views.guidelines_review, name='guidelines_review'),
path('guidelines/course/', views.guidelines_course, name='guidelines_course'),
Copy link
Member

Choose a reason for hiding this comment

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

This page is not currently listed in the admin console (not sure if this is intentional).

@tompollard tompollard merged commit a694080 into MIT-LCP:dev Mar 26, 2024
8 checks passed
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.

None yet

5 participants