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

issue#130 Update SCORM reset & backup for 4.3 structure. #140

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

joshwillcock
Copy link
Contributor

@joshwillcock joshwillcock commented Jan 16, 2024

Hi @danmarsden

Thank you for your work on this fantastic plugin. I have created a fix for #130, hopefully you're happy with with the specific code styling, I have tested this against three datasets and believe the formatting is on point.

I can only PR into an existing branch, but this obviously won't work for 4.1... If you're happy with the changes can you setup a new branch.

Thanks

@danmarsden
Copy link
Owner

Hey Josh - thanks for working on this! - I see you mention backup in the title but I can't see any changes to the backup code for the new tables - have you got those changes locally? - also I see the privacy:metadata:userid being removed in this patch but I think it's still used in the code (although it shouldn't have the text "H5P" in the string...

thanks!

@danmarsden danmarsden changed the base branch from MOODLE_401_STABLE to MOODLE_403_STABLE January 21, 2024 21:14
@joshwillcock
Copy link
Contributor Author

Hi Dan,

privacy:metadata:userid was in the lang file twice:
111 $string['privacy:metadata:userid'] = 'The user ID linked to this table.';
162 $string['privacy:metadata:userid'] = 'The ID of the user who accessed the H5P activity';

111 Works for any module, line 162 was added in issue21 when adding H5P. Given there is no benefit to having the two different, removing the duplicate alone would allow it to work in SCORM (once again).

With regards to the 'backup' element, I completely missed Moodle's course backup function as in this context I'd associated backup with backing up the SCORM tables into the recompletion_ssv / sa table... I've written the them today but I'm going to change some of the other code so SSV can use courseid as well to make the backups faster... I'll hopefully have this finished by tomorrow.

@joshwillcock joshwillcock force-pushed the issue130 branch 2 times, most recently from 04e6518 to 4d30b3e Compare January 23, 2024 14:50
@joshwillcock
Copy link
Contributor Author

Hi Dan,

Course backup & restore attached. I've edited the original changes to include the courseid on both sa & ssv to make the process easier.

Thanks

@joshwillcock
Copy link
Contributor Author

@danmarsden I believe the workflow errors were double spacing on comments related & missing a new line at the end of the file. I've corrected this and gone through all the warnings, I don't believe a single one of the remaining one is related to this patch (but existed before in other sections). If you could approval the workflow run I'll address any failures.

@danmarsden danmarsden merged commit abcc698 into danmarsden:MOODLE_403_STABLE Feb 6, 2024
7 checks passed
@danmarsden
Copy link
Owner

Thanks Josh!! - I haven't run this through heaps of testing but didn't see anything I was concerned about in the code - I'll hold off publishing this to the moodle.org plugins db until we get a few people testing it and reporting any issues they find!

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

Successfully merging this pull request may close these issues.

2 participants