Implement the "save content state" option of H5P core

[otacke]

Is your feature request related to a problem? Please describe.
When using the node.js port of H5P core, you cannot save the current content's state and resume later.

Describe the solution you'd like

• I'd like the node.js port of H5P core to call instances' getCurrentState function (if present) in regular intervals and on instances' xAPI completed/progressed statements in order to save the instances' current state, cmp. https://github.com/h5p/h5p-php-library/blob/master/js/h5p.js#L222-L251
• I'd like the node.js port of H5P core to instantiate H5P content with previously saved states (if set) within the extras argument, cmp. https://github.com/h5p/h5p-php-library/blob/master/js/h5p.js#L929 and https://github.com/h5p/h5p-php-library/blob/master/js/h5p.js#L909 and https://github.com/h5p/h5p-php-library/blob/master/js/h5p.js#L109-L111 and https://github.com/h5p/h5p-php-library/blob/master/js/h5p.js#L97-L105

Describe alternatives you've considered
People who want the "save content state" option on their platform could come up with custom solutions to implement in content types, but that would not be efficient on a global scale and defeat the purpose of H5P content being shareable across platforms.

[sr258]

Thanks for the detailled list how the PHP implementation does it. This will help making sure we follow the "specification". State support is on the roadmap. If you know anybody who needs it right away:

• You have to change generateIntegration of H5PPlayer and H5PEditor to return the appropriate values to enable state saving.
• You have to implement these endpoints:

server.post(
        '/h5p/contentUserData/:contentId/:dataType/:subContentId',
        (
            req: express.Request<
                { contentId: string; dataType: string; subContentId: string },
                {},
                H5P.IPostContentUserData
            >,
            res
        ) => {
            res.status(200).send();
        }
    );


    // STUB	    // STUB, not implemented yet. You have to get the user id through a session
    server.get('/h5p/contentUserData', (req, res) => {	    // cookie as h5P does not add it to the request. Alternatively you could add
        res.status(200).send();	    // it to the URL generator.
    });	    server.get(
        '/h5p/contentUserData/:contentId/:dataType/:subContentId',
        (
            req: express.Request<{
                contentId: string;
                dataType: string;
                subContentId: string;
            }>,
            res: express.Response<H5P.IGetContentUserData | {}>
        ) => {
            res.status(200).json({});
        }
    );

interfaces:

export interface IPostContentUserData {
    /**
     * The actual data. 0 if the data should be deleted.
     */
    data: any | 0;
    /**
     * Indicates that data should be invalidated when content changes.
     */
    invalidate: 0 | 1;
    /**
     * Indicates that data should be loaded when content is loaded.
     */
    preload: 0 | 1;
}

export interface IGetContentUserData extends IAjaxResponse {
    data: any;
}

[otacke]

@sr258 There may actually be interest in contributing the code, but it will rather not be me writing it.

[ramonziai]

@otacke That sounded promising, are there any updates on such a contribution? This is an important feature for many people, would be great to have it supported by this (really cool) port.

[sr258]

@ramonziai We've funding for the feature and it will probably be finished this year.

[ramonziai]

@sr258 Cool, great to hear!

[otacke]

@ramonziai The code exists, but for an older version of the node.js port (would need some rewrite to account for some changes that were done since forking) and it is still in review. It's nice though: features both file based and mongo db storage options for the state and uses JWT authentication/authorization to identify the user. The latter is kind of closely tied to the particular platform integration, so this would probably need to be implemented more openly - all the better that Lumi got funding to do it the way they want it right away.

[ramonziai]

@otacke Sounds great! Will it also be possible to hook in custom storage for "save content state"? For example, if one wanted to use some RDBMS, or event data stored in a Learning Record Store, or something else?

[otacke]

@ramonziai Storing/retrieving is done by implementing an interface, so you could add implementations for other storages easily, yes. But if I was not clear, sorry: The existing code has nothing to do with what Lumi will implement - although I am sure that they'll keep that in mind, too.

[JPSchellenberg]

I will start working on it today. You can review/see the progress in PR #1886

[JPSchellenberg]

I have a question to how the client handles the contentUserData and hope @otacke or someone else can help me:

There seem to be two ways to initialise the contentUserData:

1. load preloadedData from content.userContentData

2. load data via contentUserDataAjax

When I load the contentUserData via the contentUserDataAjax it does not work. The server responds with an JSON Object with a data field which contains the data as string:

When I provide it as actual JSON Object I get an error at https://github.com/h5p/h5p-php-library/blob/master/js/h5p.js#L2433

Has anyone any hints ? The content type is a Course Presentation with some SingleChoice/MultiChoice and fill in the words.

[JPSchellenberg]

And has anyone a php/wordpress instance running where I can test the behaviour of the original implementation?

[JPSchellenberg]

Update: If the contentUserData is directly integrated into the H5PIntegration on server side (method 1) it loads the state just fine.

[sr258]

Just a guess: Looks like the data is an escaped string when line 2400 is reached and nit a proper JSON object.

I use a local Drupal 7 docker container with inbuilt SQLite. It works very well but is local only. I recommend you spin one up yourself.

[JPSchellenberg]

I had the idea with a docker yesterday also. :)
The Wordpress instance doesn't use the GET request. It provides the contentUserData via the integration object.

The data must be an escaped string because it gets parsed in line 2417. If I provide a proper JSON line 2417 throws an error.

[sr258]

If you pass something to the JSON parser it must be a string, but it mustn't be escaped. That might be the problem.

[JPSchellenberg]

I tried it, but even a non escaped string does not work..

[JPSchellenberg]

Maybe it is bugged, but not noticed by many because as far as I see most integrations put the contentUserData into the Integration object.

[sr258]

I don't believe that there's a bug as serious as this in the core. Must be something about how the data is returned.

[sr258]

How can I reproduce the issue? Can I simply run the code in the PR?

[otacke]

@JPSchellenberg Sorry that I didn't reply earlier. I am currently trying to learn to work less on weekends :-)

For your case: Have you checked your implementation of the GET route for contentUserData? Are you possibly still using the stub that always resolves with {} and thus will lead to undefined (no success property, no message property)?

Beyond that: I am not sure how relevant the contentUserData GET route really is for platform integrations. The plugins by Joubel all implement it (WordPress, moodle, Drupal 7, Drupal 9), so one can use it on all of them. Still, all the plugins set the value in H5PIntegration (WordPress, moodle, Drupal 7, Drupal 9) with a default of state: {}. It's always used and the GET route never kicks in.

And if the route was used here: There is in fact an issue in H5P core. The H5P.getUserData function will resolve immediately with the value of contentUserData if it is set in the H5PIntegration object. However, it does an AJAX call otherwise and in turn H5P.getUserData will not return the return the previous state before the AJAX call is resolved.

If you now check https://github.com/h5p/h5p-php-library/blob/22115c0aeaa34faef5109c3f378add2d1647c837/js/h5p.js#L107-L137 you'll notice that the previous state may still be undefined when the H5P.newRunnable function is called, because library.userDatas is set asynchronously by the H5P.getUserData function.

[JPSchellenberg]

@JPSchellenberg Sorry that I didn't reply earlier. I am currently trying to learn to work less on weekends :-)

That's how it should be! :D

H5P.newRunnable is run before the requests resolves and I guess therefore the previousState set in the callback has no effect on the already instanced H5P. Is this correct? I think you wrote this here h5p/h5p-php-library#112 also, if I understood it correctly.

[JPSchellenberg]

How can I reproduce the issue? Can I simply run the code in the PR?

I will prepare the PR so you can test it, but keep in mind that it will be just a work in progress. I will delete the contentUserDataStorage.ts from the examples later.

[otacke]

@JPSchellenberg

H5P.newRunnable is run before the requests resolves and I guess therefore the previousState set in the callback has no effect on the already instanced H5P. Is this correct? I think you wrote this here h5p/h5p-php-library#112 also, if I understood it correctly.

Exactly, that's the reason why the previous state would not be set for a content type if you leave out the contentUserData property in H5PIntegration. H5P core would try to get the state via the contentUserData route, but the response would be too late for content instantiation in H5P.newRunnable.

Still, it might be worth to check the GET route itself (e.g. calling it in the browser with valid parameters directly). It might not return a JSON object the way that H5P expects it.

[JPSchellenberg]

@sr258 I added a commit where you can test the behaviour: ccce7a6

@otacke The structure of the returned object is

{
    data: "{"progress":2,"answers":[[["Norway"]],[{"progress":0,"answers":{"corrects":0,"wrongs":0}}],[{"answers":[]}],[[]]],"answered":[false,false,false,true]}"
}

and as far as I figured it out that should be the correct format, but I might be wrong as my understanding of the client side of H5P is very limited. :)

[otacke]

@JPSchellenberg Close. The response should match the AJAX response interface definition that's defined at https://github.com/Lumieducation/H5P-Nodejs-library/blob/master/packages/h5p-server/src/types.ts#L23-L51

You're lacking the success property, so H5P core will assume the request failed and expect the message property for more information. As mentioned before, this will give you undefined (the value of response.message) (https://github.com/h5p/h5p-php-library/blob/22115c0aeaa34faef5109c3f378add2d1647c837/js/h5p.js#L2366-L2368).

[JPSchellenberg]

I tried your suggestion, but it still does not work. So I think it is really that H5P.newRunnable is run too early.

[otacke]

@JPSchellenberg Oh, sorry, I thought I made that clear! It will definitely not work without setting contentUserData in H5PIntegration because of that issue in H5P core. Still, I was under the assumption that you wanted to implement the GET route properly.

[JPSchellenberg]

No, it was pretty clear that it won't work. I just wanted to make sure and give a feedback. :)

The question is if we want to implement something that does not really work (and is probably never called/used).
@sr258 @otacke What do you think? Should we leave that out until it is fixed in the H5P core?
I would at least make the implementation in the interface optional.

[otacke]

@JPSchellenberg I really don't know if that function is relevant (see initial comment). It doesn't seem to be used for anything except for the broken "fallback", so I assume it could be left out - if it turns out it will be used for something, it could be added easily later on. I'll reach out to Joubel in order to get some more information.

[otacke]

@JPSchellenberg Doh, I should have checked the editor first. It is actually used (https://github.com/h5p/h5p-editor-php-library/blob/7bc192798f8f6e1dee34891b56f3bf60ab320f3d/scripts/h5peditor.js#L1471) - for storing the state of wizards having run, for instance.

[JPSchellenberg]

@otacke Alright. Thank you very much! Then I will implement it.