feat(h5p-server): upgrade existing content on saving edit

[otacke]

When existing H5P content is opened in the editor of one of the original integrations (WordPress, Drupal, moodle) and there's a later version of the respective H5P content type library installed, the integrations upgrade that content when it's saved.

I am not aware whether this behavior has been changed deliberately here, but if not: I merely re-used the code of the WordPress integration here.

[JPSchellenberg]

As far as I remember this was left out as I didn't know the purpose of it.

[otacke]

@JPSchellenberg Content types may provide an upgrades.js file that contains a hook for upgrade the content's parameters from one major/minor version to another. That may be required when there are changes to semantics.json that would otherwise break existing content.

You could install an outdated version of a content type that has an upgrade hook for later versions. The later version may not be installed yet. Then create some content with that version. If you now install the later version, the existing content will continue to use the old version, while newly created content will use the later version. In order to upgrade existing content to the newer version, one could run a batch upgrade (not available for the node.js port yet) or open the existing content in the editor and then save it again. On saving, the upgrade hook will be run. That's what's implemented now.

You could, for instance, install version 1.0 Advanced Blanks first (https://github.com/sr258/h5p-advanced-blanks/tree/1.0.1). It will have the image zooming settings in the behavioural settings of the editor. Save some content.

For version 1.1 that setting was moved to the top of the editor, and that requires an upgrade hook, see https://github.com/sr258/h5p-advanced-blanks/blob/master/upgrades.js. So, you can now install the latest version of Advanced Blanks, edit your content and save it. That should run the upgrade hook. If you then edit the content again, you'll notice that the image zooming setting is now at the top and the content has kept your previous setting and still runs smoothly with the upgraded set of parameters.

[sr258]

The upgrade should definitely be run! It would be good if we added a way so that this will also work in the h5p-webcomponents package

[sr258]

@otacke Can you explain where the update is performed? I'd like to document this, as it's not obvious to me from looking at the code change.

[otacke]

@sr258 Yeah, that is hidden inside the getContent function (who wouldn't guess that given this verbose name? 😅): https://github.com/h5p/h5p-editor-php-library/blob/master/scripts/h5peditor-editor.js#L372-L388 that starts the upgrade just like when you upload content with an outdated major/minor version via the Hub (https://github.com/h5p/h5p-editor-php-library/blob/7bc192798f8f6e1dee34891b56f3bf60ab320f3d/scripts/h5peditor.js#L1603-L1676), so it will run on the client using the worker threads that you already know.

[otacke]

@JPSchellenberg @sr258 Do you happen to have a schedule for working on this ticket? Anything I can do to help? Can't review my own code, of course :-D But maybe there's something else I could do?

[sr258]

Sorry about not merging this yet. I wanted to integrate the change into the webcomponent as well, before merging, but never found the time. You could help by changing h5p-editor.ts in h5p-webcomponent so that it also has the change.

[otacke]

I'll see what I can do when.

[sr258]

@otacke I've finally found the time to have a closer look at this. I've adapted the code for the webcomponent it works in my tests. I'm not sure about the recursion protection formIsUpdated: Do you know why it is necessary at all? As the save function in the webcomponent can't be called by H5P, I think I'm save, but I want to make sure that there's isn't some edge case or content type that depends on this.

Also, do you have any old h5ps that I can use to test this in more detail (particularly nested content)?

[otacke]

@sr258 The formIsUpdated property is used to prevent submitting the same content multiple times, could be people with a nervous index finger or other measures - the whole submission process doesn't even happen if the value is true as some process seems to be running already. The property was merely copied from the original WordPress implementation. It may not serve any purpose here, and content types should have no say at that point :-)

I don't have any old content lying around unfortunately, maybe the ZUM or some other organization has some backups? If not, pulling some old Column version with one or two respective simple sub content versions (e.g. True/False and Blanks) could serve as test generators.

[sr258]

@otacke Alright, thanks for the clarification! I've renamed the variable to make it more clear. I like to divert from Joubel's naming if it makes things more clear.

[sr258]

I've just tested this and it doesn't seem to work as expected. An upgrade of Interactive Video produces a broken object (tested with the server-side rendering example at h5p-examples):

• Upload https://apps.zum.de/apps/333 to an empty system and save
• Upgrade the Interactive Video content type from the H5P Hub
• Open the video in the editor and save

The video is updated to the 1.24 but the JavaScript of the IV library throws an error as the action property of an interaction is undefined (it's the Multiple Choice interaction). Strangely, it works when I upload the .h5p file when the newer IV version is installed. In this case the upgrade is ok.

I've compared the parameters (content.json) of the broken and the correctly upgraded version and the only difference is that the interaction's action property is missing. A comparison with the original content.json file shows that the upgrade process is run, as there are many changes, even in the broken upgraded version.

I've looked at the POST request made when saving and the difference is that in the case of the broken upgrade (upgrade of already present content), the Multiple Choice content type version is left at 1.14 but in the case of the working upgrade its 1.16. (I think the semantics checker deletes the action attribute as the semantics expect 1.16) So it looks the upgrade process doesn't upgrade subcontent. @otacke Do you know why this might be the case?

[otacke]

@sr258 Sorry, no. I have never looked into the internals of the upgrade process. I might fight an hour next week to look into this.

[sr258]

@otacke I think I found the bug: The Ajax call still used the old params, not the ones received from getContent. Unfortunately, the params are in a JSON string and need to be parsed and then re-stringified. Looks like things work now.

[otacke]

@sr258 But then I'll really be able to take a couple of days off this week and will not have an excuse not to! ;-) Glad that you found the issue.

[sr258]

@otacke I've merged the PR now. Thanks for the suggestion!

[otacke]

@sr258 Cool, thanks! But I have not yet created a test!

[sr258]

@otacke It's not part of the new release yet, so there's still time. I think the feature is important enough so that it makes sense to have it even without a test. I would still very much like one, but this requires re-enabling the E2E tests (as this is all client-side code), which I disabled some time ago, because they became more and more flaky. I am planning to rewrite the old E2E tests in Cypress.IO as I picked the Puppeteer + Jest combination as there weren't any better and easier alternatives at the time. So if you are going to add tests for the upgrade it would be great if you used Crypess, too. I won't have the time to work on this before September, though.

[otacke]

@sr258 Gotcha. I've never used Cypress before (only Behat for E2E tests), but "How hard can it be?" (famous last words) ;-)