New: Migration Scripts (fixes #208) #213
Conversation
There was a problem hiding this comment.
Good work on a tricky customer @joe-replin; just added a few suggestions.
| @@ -0,0 +1,225 @@ | |||
| import { describe, whereContent, whereFromPlugin, mutateContent, checkContent, updatePlugin } from 'adapt-migrations'; | |||
|
|
|||
| describe('adapt-contrib-assessment - v2.0.0 > v2.0.3', async () => { | |||
There was a problem hiding this comment.
I think references to v2.0.0 should be v2.0.1?
There was a problem hiding this comment.
Just to double check, I have been starting mine at 2.0.0 per the discussion here. Happy to start at 2.0.1, but I don't think that's what we agreed upon.
adaptlearning/adapt-contrib-accordion#159 (comment)
There was a problem hiding this comment.
Sorry, meant to say should this use 2.0.0 as this supports framework v2 (there is no v2.0.0)
migrations/v2.js
Outdated
|
|
||
| checkContent('adapt-contrib-assessment - check assessment._questions._canShowModelAnswer attribute', async () => { | ||
| const isValid = assessments.every(assessment => | ||
| assessment._questions.every(item => |
There was a problem hiding this comment.
assessment._questions.every(item => item._canShowModelAnswer) is a little shorter
There was a problem hiding this comment.
I guess what I'm trying to achieve here is the possibility that there may be multiple assessments in a given build. So first, I want to grab all the assessments and then look for every instance of assessment._questions within each possible assessment. Most of the time there will only be 1 assessment, but we do allow multiple.
There was a problem hiding this comment.
Why is !== false necessary when item._canShowModelAnswer would be a truthy?
migrations/v2.js
Outdated
| return true; | ||
| }); | ||
|
|
||
| updatePlugin('adapt-contrib-assessment - update to v2.1.0', { name: 'adapt-contrib-assessment', version: '2.1.0', framework: '>=2.0.18' }); |
There was a problem hiding this comment.
The update to v2.1.0 requires framework v2.1.3.
There was a problem hiding this comment.
Good looking out. Thanks.
migrations/v2.js
Outdated
| }); | ||
|
|
||
| checkContent('adapt-contrib-assessment - check assessment._requireAssessmentPassed attribute', async () => { | ||
| const isValid = assessments.every(assessment => |
There was a problem hiding this comment.
Just to go with convention, perhaps: const isValid = assessments.every(assessment => !_.has(assessment, '_requireAssessmentPassed'));
There was a problem hiding this comment.
Thanks. Not super familiar with lodash helpers. But will apply this to other, similar delete modifications.
migrations/v2.js
Outdated
| return true; | ||
| }); | ||
|
|
||
| updatePlugin('adapt-contrib-assessment - update to v2.1.1', { name: 'adapt-contrib-assessment', version: '2.1.1', framework: '>=2.0.18' }); |
There was a problem hiding this comment.
The update to v2.1.1 requires framework v2.2.
migrations/v3.js
Outdated
| }); | ||
|
|
||
| checkContent('adapt-contrib-assessment - check course._postTotalScoreToLms attribute', async () => { | ||
| const isValid = !assessmentConfig._postTotalScoreToLms; |
There was a problem hiding this comment.
const isValid = !_.has(assessmentConfig, '_postTotalScoreToLms');
migrations/v4.js
Outdated
| }); | ||
|
|
||
| describe('adapt-contrib-assessment - v4.3.0 > v4.4.0', async () => { | ||
| let course, assessmentConfig, assessments; |
There was a problem hiding this comment.
assessments isn't initialized in this block
There was a problem hiding this comment.
What about the _scoreToPass and _correctToPass where it is used?
migrations/v4.js
Outdated
| /** | ||
| * * Add JSON field and set attribute. | ||
| */ | ||
| mutateContent('adapt-contrib-assessment - add assessment._scoreToPass', async () => { |
There was a problem hiding this comment.
should this have been _correctToPass and on assessmentConfig?
| whereContent('adapt-contrib-assessment - where assessment', async content => { | ||
| course = content.filter(({ _type }) => _type === 'course'); | ||
| assessmentConfig = course.find(({ _assessment }) => _assessment); | ||
| if (assessmentConfig) return true; |
There was a problem hiding this comment.
I think the _assessment configuration in course is optional, so we would still want to run the migrations on the _assessment configurations in articles
There was a problem hiding this comment.
All of the mutations taking place within this describe statement is at the course level per the previous comment about switching _correctToPass and _scoreToPass to assessmentConfig.
migrations/v4.js
Outdated
| /** | ||
| * * Add JSON field and set attribute. | ||
| */ | ||
| mutateContent('adapt-contrib-assessment - modify assessment._questions._resetType value', async () => { |
There was a problem hiding this comment.
I think your caution is justified; as with not changing boolean defaults, we should only change enumerated types if the default becomes invalid (and the invalid value is used). I would therefore remove this migration.
migrations/v2.js
Outdated
| whereContent('adapt-contrib-assessment - where assessment', async content => { | ||
| articles = content.filter(({ _type }) => _type === 'article'); | ||
| assessments = articles.filter(({ _type, _assessment }) => _type === 'article' && _assessment !== undefined); | ||
| return assessments.length; | ||
| }); |
There was a problem hiding this comment.
We could probably simplify this further to a single filter.
Thoughts on changing assessments to assessmentArticles?
| whereContent('adapt-contrib-assessment - where assessment', async content => { | |
| articles = content.filter(({ _type }) => _type === 'article'); | |
| assessments = articles.filter(({ _type, _assessment }) => _type === 'article' && _assessment !== undefined); | |
| return assessments.length; | |
| }); | |
| whereContent('adapt-contrib-assessment - where assessment', async content => { | |
| assessmentArticles = content.filter(({ _type, _assessment }) => _type === 'article' && _assessment !== undefined); | |
| return assessments.length; | |
| }); |
migrations/v4.js
Outdated
| const isValid = assessments.every(assessment => | ||
| assessment._scrollToOnReset === false | ||
| ); |
There was a problem hiding this comment.
| const isValid = assessments.every(assessment => | |
| assessment._scrollToOnReset === false | |
| ); | |
| const isValid = assessments.every(({ assessment }) => | |
| assessment._scrollToOnReset === false | |
| ); |
There was a problem hiding this comment.
Added in a separate commit.
Co-authored-by: joe-allen-89 <[email protected]>
Co-authored-by: joe-allen-89 <[email protected]>
Co-authored-by: joe-allen-89 <[email protected]>
Co-authored-by: joe-allen-89 <[email protected]>
Co-authored-by: joe-allen-89 <[email protected]>
Co-authored-by: joe-allen-89 <[email protected]>
Co-authored-by: joe-allen-89 <[email protected]>
Co-authored-by: joe-allen-89 <[email protected]>
New
Testing
Test using this branch of the Adapt Framework which includes the scripts that handle the migration.
adaptlearning/adapt_framework#3633