Refactor the default completion and duration content completion optio… #4182
Conversation
…ns to be in utils.js, and make the default data visible in the resource panel
There was a problem hiding this comment.
Thank you @marcellamaki. When it comes to code, I think overall it's a great improvement and feels much more clear and robust than before. Also thanks for documenting new helper functions.
I left a few comments. Nothing is blocking. I'd welcome if those related to audio/video logic and covering it with one or two unit tests could be addressed, but it'd be also fine to do it as a follow-up.
contentcuration/contentcuration/frontend/channelEdit/components/ResourcePanel.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank you for the new updates @marcellamaki, overall it's looking fine. The only thing that'd be good to do before merging is to remove console.logs (not sure why linter checks didn't catch them?), rest of my new review is not blocking. It's already a great improvement toward reducing tech debt.
| @@ -336,17 +332,19 @@ export function getCompletionCriteriaLabels(node) { | |||
| duration: '-', | |||
| }; | |||
|
|
|||
| console.log('filesss', files); | |||
There was a problem hiding this comment.
Can be remoooved :) Also applies to other console.log occurrences in this file
| }); | ||
| }); | ||
| }); | ||
| describe(`for exercises`, () => { |
There was a problem hiding this comment.
It'd be good to place this outside of the describe('getCompletionCriteriaLabels') block, because it doesn't test, neither describe behavior of getCompletionCriteriaLabels function like other tests cases but rather that of getCompletionDataFromNode. This applies to another new test case below as well.
A nice way to try out if tests organizations and descriptions make sense from the documentation point of view is to run the test suite with --verbose flag and read through the output
yarn test-jest channelEdit/__tests__/utils.spec --verbose
And this is nitpick (squared), on the screenshot above you can also see that it may be nice to distinguish between audio and video test cases descriptions to be able to say which one corresponds to which as right now, their names are identical. If one of them failed, ideally we'd be able to say immediately if it fails for video or for audio without digging deep into the tests code.
| export function getCompletionCriteriaLabels(node) { | ||
| if (!node) { | ||
| export function getCompletionCriteriaLabels(node = {}, files = []) { | ||
| if (!node && !files) { |
There was a problem hiding this comment.
Noting that if node = {} then !node is false, and if files = [] then !files is also false.
| const { model, threshold } = node.extra_fields?.options?.completion_criteria || {}; | ||
| const masteryModel = threshold?.mastery_model; | ||
| const completionCriteria = node.extra_fields?.options?.completion_criteria | ||
| ? JSON.parse(JSON.stringify(node.extra_fields?.options?.completion_criteria)) |
There was a problem hiding this comment.
Why was the stringify and parse required?
Summary
Description of the change(s) you made
This PR refactors the default completion and duration model logic - previously only used in the EditModal to help with
Manual verification steps performed
Screenshots (if applicable)
There are a great deal of permutations, so I have not included them all, but a few samples -
Does this introduce any tech-debt items?
I don't think so! Hopefully it rather helps reduce it.
Reviewer guidance
How can a reviewer test these changes?
For code review:
yarn run devsetupand see the default completion and duration values for the different content types (note that there are not default 'duration' values for anything other than audio/video content)Are there any risky areas that deserve extra testing?
The EditModal should be fully regression tested by QA team to ensure it is still working as expected.
References
Fixes #4180
Comments
I have tried to keep this as simple as possible for release - there might be other optimizations we could add later (i.e. do we want an empty duration to display at all by default?) but that did not seem to be critical at this point.
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)