New: Migration Scripts (fixes #158) #159
Conversation
| describe('adapt-contrib-accordion - v1.0.0 > v2.0.0', async () => { | ||
| let accordions; | ||
|
|
||
| whereFromPlugin('adapt-contrib-accordion - from v1', { name: 'adapt-contrib-accordion', version: '<=2.0.0' }); |
There was a problem hiding this comment.
@joe-replin should this be <2.0.0?
There was a problem hiding this comment.
Interesting question. I'm migrating to v2.0.0 so I guess I'm migrating from everything from v1.0.0 and up. I'm guessing I shouldn't be attempting to migrate from below v2.0.0 and my migrations should start at the point in which I am migrating v2.0.0 > v2.0.4? Is that the consensus?
There was a problem hiding this comment.
The describe block should not run if already at v2.0.0 hence < instead of <=
There was a problem hiding this comment.
I agree. My question is whether we should include describe blocks for changes introduced in v2.0.0, or if we should start our migrations from the first version after v2.0.0. In this case, that would be v2.0.4.
There was a problem hiding this comment.
Cool, I don't know. @joe-allen-89 any thoughts?
There was a problem hiding this comment.
The pattern I've followed in all my scripts so far is as follows.
If there is some v2.a.b to v2.c.d that has an attribute change then that will go in v2.js. So I'd say yes if there is some attribute change from v2.0.0 to v2.x.x then include it in v2.js.
On the other hand if an attribute change occurs between the last release of v2 and, say, v3 then I would put the migration in v3.js because the change happens when you move to v3.
There was a problem hiding this comment.
Not that I can see many people still on v1 versions but it's probably best we don't try and migrate anything lower than the first script.
I think the best course of action is to create a range rather than just use < for the first script to prevent migrating older versions, the two options would be:
whereFromPlugin('adapt-contrib-accordion - from v1', { name: 'adapt-contrib-accordion', version: '>=1.1.4 <2.0.0' });
or remove the describe for 2.0.0 and change the following whereFromPlugin to
whereFromPlugin('adapt-contrib-accordion - from v1', { name: 'adapt-contrib-accordion', version: '>=2.0.0 <2.0.4' });
As 1.1.4 doesn't have a FW version listed I'd be inclined to go for option 2.
There was a problem hiding this comment.
So just to be clear, we shouldn't be supportting v1 FWs. If I went with your option 2, I would remove everything from this from from line 4 - line 108. And then we would start where the migration goes from v2.0.0 > v2.0.4.
migrations/v2.js
Outdated
| }); | ||
|
|
||
| checkContent('adapt-contrib-accordion - check accordion._shouldCollapseItems atrribute', async () => { | ||
| const isValid = accordions.every(({ _classes }) => _classes !== undefined); |
There was a problem hiding this comment.
copy and paste error - _classes should be _shouldCollapseItems
migrations/v5.js
Outdated
| mutateContent('adapt-contrib-accordion - update accordion._supportedLayout attribute to full-width', async () => { | ||
| accordions.forEach(accordion => { | ||
| /** | ||
| * ? Would this potentially break the layout of any course being migrated? |
There was a problem hiding this comment.
I would say there's a good chance of that. half-width is still a valid layout. Perhaps only set if _supportedLayout does not have a valid value?
migrations/v5.js
Outdated
| return true; | ||
| }); | ||
|
|
||
| checkContent('adapt-contrib-accordion - check accordion._supportedLayout atrribute is now full-width', async () => { |
There was a problem hiding this comment.
The check should be that _supportedLayout is one of full-width, half-width or both
migrations/v5.js
Outdated
| return true; | ||
| }); | ||
|
|
||
| updatePlugin('adapt-contrib-accordion - update to v2.0.4', { name: 'adapt-contrib-accordion', version: '2.0.4', framework: '>=5' }); |
There was a problem hiding this comment.
Needs a version tweak
migrations/v7.js
Outdated
| @@ -0,0 +1,109 @@ | |||
| import { describe, whereContent, whereFromPlugin, mutateContent, checkContent, updatePlugin } from 'adapt-migrations'; | |||
|
|
|||
| describe('adapt-contrib-accordion - v5.3.0 > v7.0.0', async () => { | |||
There was a problem hiding this comment.
_imageAlignment came in v7.3.0 rather than v7.0.0
migrations/v7.js
Outdated
| /** | ||
| * * Add field to each item in a JSON array and set attribute. | ||
| */ | ||
| mutateContent('adapt-contrib-accordion - add accordion._items._imageAlignment', async () => { |
There was a problem hiding this comment.
This was done in the previous migration?
There was a problem hiding this comment.
Yes. I have deleted the duplicate.
Waiting for confirmation.
|
@chris-steele, appreciate the early review. I am requesting another review from you and have officially turned this PR to 'Needs Reviewing'. |
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]>
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]>
|
Thank you for the thorough review, @joe-allen-89. Looking to resolutions by @chris-steele and and answer to a question from @oliverfoster: #159 (comment). |
New
Testing
Test using this branch of the Adapt Framework which includes the scripts that handle the migration.
adaptlearning/adapt_framework#3633