-
Notifications
You must be signed in to change notification settings - Fork 20
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 --> implement take training feature and resume training feature for user #1951
create training basics --> implement take training feature and resume training feature for user #1951
Conversation
3a7bb55
to
4bf486d
Compare
7a3246d
to
662d905
Compare
Quick Note: There is a weird bug i just noticed Currently OnPlatform training is treated unique based on version_number and training_type. So if a user had taken a training with version 1.1, and then next month there is a minor update with version 1.2, their previous training would still be valid. But if they go and they try to review the training again, they will be retaking the new version 1.2 of training.(because by default we always provide them with the latest version of training). Which brings to the weird bug which is that if the user hadn't finished the training 1.1 and the version 1.2 was lunched, then when they try to resume the training, it will be like taking the training from scratch for them |
9c4f330
to
905f170
Compare
I merged this PR on top of #1950 to do some testing. There were a few issues I came across while testing:
Minor thing, but it is currently possible to upload duplicate training courses at:
Reloading the page after following the points above triggers the following error. Returning to http://localhost:8000/settings/platform-training/3/ and clicking "Continue" triggers the same error.
After completing a course, returning to http://localhost:8000/settings/platform-training/3/ and clicking "Review" triggers the same |
0913bd7
to
d4d94ec
Compare
Addressing the comments by @tompollard
The course addition & usage functionality should be working. Please check and let me know. |
…uploading json (#1950) ## 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.
3411cdf
to
673b3c3
Compare
Thanks for running through this today @Rutvikrj26. As discussed, please could you remove all javascript elements as far as possible? If dynamic features are needed, consider HTMX. |
This adds test coverage for most of the URLs in the `project` app. See the previous pull requests #1695, #1922, #2108 for background. With this change, the bulk of site functionality should now be included in the TestURLs framework. A few small apps are left to add: - `oauth` (should be easy) - `sso` (should be easy but I'm not sure this is enabled in test configs) - `training` (awaiting pull #1951) And a number of URLs across the site are skipped due to a lack of fixture data: - `console` - `edit_submission` - `copyedit_submission` - `awaiting_authors` - `publish_submission` - `event_agreement_detail` - `event_agreement_delete` - `event_agreement_new_version` - `project` - `serve_document` - `published_project_request_access` - `search` - `redirect_challenge_project`
@bemoody I've updated the URL for the heading and the PR is ready for further review / merge. |
Versions of a course are immutable by design (a course version that has been published and taken by users are certified for that particular version of the course - and hence once published on the platform, it cannot be - updated) May be I can rename the button to
This was abstracted out to a different PR #2223 as it dealt with updating the certifications page and had an updated in the queryset logic. It is a followup PR on this, and will be addressed in it.
This sounds like a good idea - I'll update the #2223 with this as a requirement.
I haven't been able to replicate the error.
Yes, the review is just to go through the text once again to brush up anything if the user wants. Given that these are going to be basic mandatory courses on it security or something similar, it doesn't have any real purpose.
The Expiry date is the date the admin chooses on the date field on the side of the expire button. I can add a new column of date for the archived courses and then add a line at the top of the archived table detailing that the trainings allotted to the people who took that version of the course will no longer be valid after the expiry date listed. And yes, a new issue/pr sounds better for that, as this one is long as it is.
Agreed, this looks a bit misleading. It should rather say open the training page, where you can then choose to start/continue/review the training modules. |
1b8a9f2
to
0fcc163
Compare
@tompollard I've made the changes you requested on switching from expiring to archiving and simplifying the logic. However, there is a weird issue I am facing in the tests. The test keeps giving 404 error - which it technically shouldn't. The two url endpoints failing in the tests are: But these work perfectly fine when I open them in my desktop. I tried figuring out what's going on, but couldn't figure out the error. One reason of something like this happening is the training element not getting loaded up - but the loaddemo command loads up the demo training when I test locally. |
@Rutvikrj26 odd, it's not immediately clear to me why the tests are failing. i added a breakpoint at: physionet-build/physionet-django/physionet/test_urls.py Lines 191 to 194 in 75b7f91
...to catch the error. The url and parameters appear to be set correctly (
|
@Rutvikrj26 It looks like the issue is that the course is not active. If you drop a breakpoint() around
Then run:
You'll see that |
@tompollard I've addressed all the comments and updated the fixtures to have a fallback version of the course so that even after archiving tests works fine. |
@tompollard I need this PR to be merged to complete the work on certifications page #2223. Please merge it if everything looks okay. |
@tompollard @bemoody There is a weird error in the Testing where it fails while running Docker commands saying docker-compose is command is not available. Any recent changes that might have triggered this? |
We are seeing a new "`docker-compose: command not found`" error on the [docker-build-test.yml github workflow](https://github.com/MIT-LCP/physionet-build/blob/dev/.github/workflows/docker-build-test.yml). See: #2268 It looks like "`docker-compose`" is being phased out, in favour of "`docker compose`": https://docs.docker.com/compose/migrate/. This pull request switches all mentions to `docker-compose` to `docker compose`. Merging this pull request should fix the error in: #1951
@Rutvikrj26 Should be fixed by #2269. Please could you rebase on the dev branch? |
6282a5d
to
0630a93
Compare
@tompollard Looks good to merge. |
@Rutvikrj26 it looks like the user migrations are stale (0061_cloudinformation_aws_userid_and_more.py collides with 0061_training_course.py). Please could you regenerate the migrations so we can go ahead and merge? This is also an opportunity to rebase and clean up commit history if you'd like to do that. |
Here users should be able to see their progress and basic training details, modules, etc implement basic features trainings for the user This commit allows users to take the test from settings. The quiz.html file loads all the content and questions for a module based on the order. All the data is hidden from user and gradually displayed as users go through the training We have two level of verifications of answers. the first one is done on quiz.html 1. When users go through the content and they will see the related questions and answer the questions. Users can go to next section or skip the question only if they have answered the question correctly previously. 2. We also have another verification done on view. Here if the users had correctly answered the questions a json string is generated at the end of module and sent via POST, we will then check the database to verify that the data in json string(question,answer) and match it with the database. This should help us stop someone from just sending a post request to complete the training. How the resume works? 1. When users go through the training module, we will track the contents, quiz that they have completed and store it in the database. 2. If the user clicks Next after completing a section or quiz, we will check if it was completed in previous session, 1. If not we will send a post request to the server to update the database, and then load the next section or quiz. 2. If yes we will just load the next section or quiz. 3. One the user reaches the end of the module, we will send submit the form and the server will check if the user has completed all the sections and quizzes. If yes, we will update the database and redirect the user to the training dashboard. Quick note: 1. Only multiple choice questions is supported yet. Add Training to user settings tests for training/courses Fully working version to take online-trainings Fully working version to take online-trainings Updated the Queryset logic to use the course's valid_duration for the determining of the training is valid or expired. Fixing test_views.py to switch from name to title Updated the query to no use union - failing a lot of tests Fixing the tests for Update logic change from courses view to course_details view Updated the test to adjust for schema change & the new updated schema. Fixing styling changes Reverting changes in training status querying logic - extracted into a seperate PR Updated the UI & forms to show active courses, and only showup when there are active courses on the platform Changed the location of the urls Switched the existing dynamic implementation to stateful implementation. Fixing the module-progress error Removed remaining debug statements Removing redundant views remaining from initial dynamic behavior. Removed redundant url from training Removed redundant view imports from user.urls Updated the course-training connect from M2M to 12M Updated the views and methods according to the new 12M model between training and courses Updated the last_completed_order to have a default value of 1 rather than 0 remove redundant test views Fixing the trainig tests Added basic tests for new views Fixed styling issues Fixed the issues with new tests Added TEST_DEFAULTS & TEST_CASES for training app Added _user_ for logged in training Added _user_ for logged in training added course_progress to the demo_training fixture updated the training test urls to skip post requesrts updated url endpoint to the correct training page updated the template and test view updated the template to conditionally show the line on training Improving the queries to use inbuilt django commands for hanling checks and deletions updated module_order in test cases fxing styling issues refactoring the url name to match the view name (WIP) rebased onto dev & fixed leaf migration fixing the views for errors mentioned in PR comments Addressed the issues raised in the comments and fixed the views to handle requests more efficiently. updated the tests to match with the updated url routes Added a unique constraint on the completed content to fix the applciation crashing on multiple attempts. Cleaned up the views.py to remove unused imports and residual function imports updated the function to fix more than required imports Update the console to address tom's comments updated the training page to say open training rather than start training Updated the view logic to fix the crashing behavior on start Updated the expiring functionality to archiving switched the model method name Fixed the code to cleanup left-out files and added docstrings cleaning up the accidentally pushed changes to test_urls.py added another course version and enabled the testing for archive Added docstrings for the views.py Fixed styling issue. Removed migrations Revert "Removed migrations" This reverts commit 67c31ce93d29abbe4f90d78b4ef9d8539599d994. Removed migrations removed the merge for 61/52
0630a93
to
2f4e7b0
Compare
@bemoody @tompollard I've squashed the commits and converted it to one - plus added migrations separately. A quick question - The |
@Rutvikrj26 in case you didn't receive the notification, tests are failing. |
@tompollard I'm not looking to get this one merged yet. I saw the tests are failing. The comment was to ask if you have an idea of what could be causing this, as I am not able to replicate the command failure locally. |
Oh sorry, I'd missed the earlier question. I'll try to take a look shortly/ |
I think I just fixed it. I had generated migrations but for some reason, it did not get added to tracking files. This should fix the issue. |
Easy to miss new, untracked files! |
@tompollard The PR is now finally ready to be merged! |
What?
Here i have added a feature that will allow users to take the training from settings. User can choose from a list of available
trainings and take the onplatform training. Users can also see the progress of the training and can resume the training from where they left off.
Why?
This is a follow up PR for #1950
How?
Here we create 3 new views to handle the training.
1.
training.views.take_training
This view will take care of training homepage. It will show progress of the training if the user has already started the training, show them details of the training and allow them to start/resume and review the training.
2.
training.views.take_module_training
This view will take care of letting users take individual modules of the training. It will allow users to take the module from where they left off.
Users will need to finish module1 before they can start module2 and so on.
Here we have used a considerable amount of javascript to make the training interactive. How it works is when the user loads the module page,
we initially load all the content, quizzes and order them based on the
order
field, and initially we hide all the content and quizzes.Then we show either the first content,quiz or the next one after a content, quiz that they had completed last time. and we keep doing this until the user has completed the module.
3.
training.views.update_module_progress
Testing? [WIP]
Following tests will be added
Screenshots (optional)
Anything Else?
How to take a training as a user?
Settings