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

Completion duration updates #3777

Merged

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Oct 27, 2022

Summary

Description of the change(s) you made

These changes are to ensure that data is persisted correctly in the front and back end, and there are no "invisible" validations that cause data to not be saved as expected.

edited: This has been implemented and checked for: video, audio, document, html5, and practice quizzes

Manual verification steps performed

For each file type listed above

  1. Add a file
  2. Ensure all required field are filled out
  3. Save
  4. Re-open edit modal and ensure completion/duration was saved
  5. Move on to next completion/duration option (as applicable). Save, and repeat process.

Does this introduce any tech-debt items?

Tests still need to be finalized for this PR. It has been opened just for the sake of timely manual QA.


Reviewer guidance

How can a reviewer test these changes?

See manual verification steps above.

Are there any risky areas that deserve extra testing?

Any notable problem areas from open issues, especially:

  • When time spent is equal to duration is selected
  • Determined by resource

across all relevant content types.

Comments

When testing on the dev server, the audio and video default files that are generated for engineers (yarn run devsetup) are both 00:00 long. This does cause an error, as for audio and video files, there is a required minimum in the validation to be > 0. I am not sure if these files are in the usual manual QA workflow, but please do note that mp3 or mp4 files that are 0 seconds long will fail to save. I will check in with @rtibbles to see if we should handle this in a different way, or if it too small of an edge case to consider.


Contributor's Checklist

PR process:

  • If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
  • If this includes an internal dependency change, a link to the diff is provided
  • The docs label has been added if this introduces a change that needs to be updated in the user docs?
  • If any Python requirements have changed, the updated requirements.txt files also included in this PR
  • Opportunities for using Google Analytics here are noted
  • Migrations are safe for a large db

Studio-specifc:

  • All user-facing strings are translated properly
  • The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
  • All UI components are LTR and RTL compliant
  • Views are organized into pages, components, and layouts directories as described in the docs
  • Users' storage used is recalculated properly on any changes to main tree files
  • If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

  • Code is clean and well-commented
  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Any new interactions have been added to the QA Sheet
  • Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

Approving so we can continue with testing.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Some questions - not clear if they're about your implementation or the spec itself!

@@ -324,6 +323,13 @@
model: CompletionCriteriaModels.REFERENCE,
threshold: null,
};
} else if (value === CompletionDropdownMap.completeDuration) {
// set to '1' as the minimum here, due to validation rules requiring > 0
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand why we're setting a default here - feels like if there is no value, we need to let the user set it.

@rtibbles
Copy link
Member

Merging this for now for further QA testing. There is still an extant issue with duration setting for exact duration, but we can fix that in follow up.

@rtibbles rtibbles merged commit 4d45824 into learningequality:hotfixes Oct 28, 2022
@pcenov
Copy link
Member

pcenov commented Oct 28, 2022

@marcellamaki Here are the issues that I was able to identify:

Issue Screenshot
Audio/Video - If I save a resource of type .mp3/.mp4/ webm/html5 as 'When time spent is equal to duration/Short activity' then when I edit it back to 'When time spent is equal to duration/Time to complete' it is displayed as incomplete. If I exit the editor and then come back to edit the resource I can see that the setting is reverted back to 'When time spent is equal to duration/Short activity' https://user-images.githubusercontent.com/79847249/198594299-276ec73c-4884-43ea-a12e-24204dbaa49e.mp4
:-------------------------: :-------------------------:
Document/Exercise - Note that the default value is 'Viewed in its entirety' and Duration is empty with explanation that it's an optional field. 1. If I start changing the values for completion and duration then I am no longer able to get back to the default state, the Duration field has a value of Time to complete and the explanatory text that it is optional is also no longer visible. 2. When I first select 'When time spent is equal to duration/Time to complete there is a prefilled default value of 1 minute 2022-10-28_16-40-39
:-------------------------: :-------------------------:
ZIP - Here the default value in the Completion ddl is 'When time is equal to duration' and the Duration field is not prefilled while being required. At the same time if I just go ahead and save it the resource is not marked as incomplete. This is also the case for imported old resources without a Duration value. https://user-images.githubusercontent.com/79847249/198625339-6e4605a1-8dae-43f1-b0ac-393e2865e744.mp4
:-------------------------: :-------------------------:
Is the specified Duration value actually supposed to be working when the resource is imported in Kolibri? I tried with several resources unsuccessfully. https://user-images.githubusercontent.com/79847249/198634639-752e7c00-efd1-454a-a662-df9c0f6c4413.mp4
:-------------------------: :-------------------------:
Exercise - If the values for M of N are left empty and the user decides to switch the Completion to 'Practice quiz' then the resource is marked as Incomplete without clear indication to the user why this is so https://user-images.githubusercontent.com/79847249/198641118-b2ac7a9f-67fe-4eaa-a5cf-7e54c6795966.mp4
:-------------------------: :-------------------------:

@marcellamaki
Copy link
Member Author

Thank you @pcenov! I will look into all of these

@marcellamaki marcellamaki mentioned this pull request Oct 28, 2022
24 tasks
@bjester bjester mentioned this pull request Nov 9, 2022
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.

4 participants