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

Merge branches 'stable-catalyst-400' and 'stable-catalyst-fix-resize' into stable-catalyst #67

Merged
merged 40 commits into from
Feb 12, 2025

Conversation

Fragonite
Copy link

@Fragonite Fragonite commented Jan 6, 2025

TL;DR

Consolidates the active branches and updates the minified js.

Why are there so many active branches?

The stable-catalyst-400 branch was created for Moodle 4.0+ compatibility around April 2023. It looks like it was intentionally backwards compatible with earlier versions and intended to get merged back into the default branch, stable-catalyst.

In December 2023, stable-catalyst was merged into stable-catalyst-400 instead of the other way around. It is likely some clients were now on the 400 branch and consolidation was not a priority. The plugins continued to diverge, with some developers working on one branch and some the other.

In any case, it's not immediately clear that the 400 branch is active or the most developed given the dozens of other branches in the repo.

Moodle 4.0 and prior are now out of support so compatibility differences between branches are not much of a concern. The main goal is to update the default branch and prevent further split development effort.

The fix-resize branch was a temporary solution for a client and has proven stable.

What will happen to the stable-catalyst-400 branch?

Code freeze and eventual removal. New commits get merged into the default branch.

Testing

Run php vendor/bin/phpunit --testsuite mod_hvp_testsuite and verify the tests pass.

A full code review is not expected, there are no new commits here (apart from grunt, supported version increase, and code checker fixes). You may want to try merging the branches and resolving conflicts yourself and check you get the same result.

The most relevant PRs are:

Upstream

Ideally we want to just use upstream, or at least submit PRs to upstream.

Our fork has accumulated a fair few changes, we would need to go through them and split it up into suitable issues and PRs.

rhell4 and others added 30 commits April 13, 2023 10:59
Add height if the activity has description
…atalyst-400

Resolve merge conflicts stable catalyst 400
* feat: Add support for downloadable H5P via Mobile app

* feat: updated cache method, handles video

* feat: handle elements inside of canvases

* bugfix: fix cron handler context

* bugfix: fix background image url detection

* feat: add margin on right so user can always scroll

* bugfix: force only ttf fonts for ios compatibility

* bugfix: fix default ios serif font

* feat: match iframe scrollheight to stop inner scrolling

* chore: add more docs

* docs: update mobile docs

* cleanup: move all hvp data to new window namespace

* bugfix: add unique id to selectors to mitigate mobile page load race conditions

* cleanup: put resizer on window namespace

* bugfix: run hvp only after cached assets are loaded

* bugfix: mobile resizer handle infinite height content types

* bugfix: start completion manager only after H5P is accessible

* docs: update mobile docs

* bugfix: remove resizer, opt for fixed height instead

* bugfix: fix resizer properly

* bugfix: fix unit test
chore: fix namespacing issues in 4.2+
This includes:
- Bulk update libraries to latest versions
- Bulk upgrade content to latest library version
- Bulk export libraries (for importing to another site)
- Bulk import is already a feature using the existing "Upload Libraries"
Increase php time limit for library export/import
Resolves 4.5 compatibility - install failure h5p#562
Replaced MESSAGE_DEFAULT_LOGGEDIN + MESSAGE_DEFAULT_LOGGEDOFF to MESSAGE_DEFAULT_ENABLED as per Moodle plugin upgrade documentation: https://github.com/moodle/moodle/blob/13c12756b4d7e85f2d2e34038216179fd287c9c2/lib/upgrade.txt#L716

Thanks @otacke and @lkcivan
@Fragonite Fragonite requested review from dmitriim and removed request for dmitriim January 6, 2025 04:42
@Peterburnett
Copy link
Collaborator

This looks good, and is good to consolidate. There are some syntax errors inside of the CI that should probably be addressed, but other than that, its great to consolidate our supported version and create a more maintainable fork for our heavily customised clients

@Fragonite Fragonite force-pushed the merge-400-into-stable-catalyst branch from 774e8e8 to f604d42 Compare February 7, 2025 06:14
@Fragonite
Copy link
Author

Hi @Peterburnett,

Thank you for your feedback. I have pushed a commit resolving all code checker errors if you want to take another look.

Cheers.

@Peterburnett
Copy link
Collaborator

Hi @Fragonite

The latest commit doesn't look quite correct. The proxy script exists to allow for inflight modification of resources as required, however it should NOT assert anything about the users login status. The user's cookies are passed through to the end resource called, where their identity is verified, however this should NOT be done at the proxy level. This will prevent HVPs on guest courses or on other unauthenticated pages from working.

@Fragonite Fragonite force-pushed the merge-400-into-stable-catalyst branch from ef7d8bd to f604d42 Compare February 11, 2025 05:59
@Fragonite
Copy link
Author

Hi @Peterburnett,

Thank you, I have removed the commit.

Cheers.

@Fragonite Fragonite merged commit 86be024 into stable-catalyst Feb 12, 2025
14 of 56 checks passed
@Fragonite Fragonite deleted the merge-400-into-stable-catalyst branch February 12, 2025 03:13
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.