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

Merging H5P integration object properties should include new properties #1296

Closed
0xMurage opened this issue Apr 13, 2021 · 2 comments
Closed
Assignees

Comments

@0xMurage
Copy link

Libraries ,browser, and operating system info:

  • Operating System: Linux
  • NodeJS Version: 14
  • @lumieducation/h5p-server: 7.2.0
  • @lumieducation/h5p-webcomponents: 7.2.0

Issue description:

We are using the standalone player on our Wikonnect platform (single page application) and also @lumieducation/h5p-webcomponents package for the editor.

I have noticed when a user visits the standalone player, it sets the following H5P global integration properties.
Screenshot from 2021-04-13 11-43-24

When the user later visits the H5P editor page, the merge utility function of the library

export function mergeH5PIntegration(

does not include the missing new properties as below

Screenshot from 2021-04-13 11-45-44

The above merged H5PIntegration object not include hubIsEnabled property.
This will signal the H5P core editor library to make an Ajax GET request for the available local content-type libraries as per this LOC

Thus,
i) can the H5P integration merge function include all new properties on resultant window.H5PIntegration object

ii) or the GET /ajax?action=libraries endpoint fallback to retrieving all libraries information if machineName, + majorVersion+minorVersion parameters don't exist?

@sr258
Copy link
Member

sr258 commented Apr 13, 2021

Thank you for reporting this.

It would be easy to simply copy over the hubIsEnabled property, but I guess there are more properties that must be merged into the object if the H5PIntegration object comes from a different system. The current implementation assumes that H5P is only used with our implementation and that's why the merge is only partial.

My idea would be to use a deep merge library to simply merge the complete integration objects, except for the contents property, in which I would replace the whole content objects, to avoid issues if you switch from player to editor for the same content id. I think it would be safe to do that... Any thoughts @mimidotsuser ?

@0xMurage
Copy link
Author

@sr258 using deep merge for the properties (except for the contents property) is perfect and predictable.

@sr258 sr258 self-assigned this Apr 25, 2021
@sr258 sr258 closed this as completed in 841c360 Apr 25, 2021
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

No branches or pull requests

2 participants