Bumping minor for this so we can remove some old stuff (mainly or only update hooks).
Indeed.
Closing in favor of
📌
Improve module code quality checks
Active
and going for it.
As you can see here: https://xapi.com/statements-101/, statements should have the elements that are reported as missing. But I guess there can be different cases so this also needs more invastigation.
We haven't tested with Moodle, the statement data validation happens in Drupal\lrs_xapi\XapiStatementStorage::setStatement() and that's where the errors come from so you need to do some debugging what is is that actually Moodle sends there and compare it with Xapi specs.
In any case if those statements are compliant with the Xapi standard, I'll be happy to review and merge any improvements included.
It's a storage-only LRS at the moment, there's no statement UI, only API where you can store and retrieve statements.
Thanks for testing!
Merged without any testing feedback other than my own so it doesn't hang for too long.
Ok, maybe if we placed it in some folder other than .ddev, otherwise there'll be conflicts.. I can't think of a good solution at the moment though.
🙏
Sorry, it’s going to annoy me every time I run ddev commands on the platform when in the lms folder.
Please use https://github.com/graber-1/drupal_lms_ddev instead.
If someone else will be doing more work on this module they can decide wether to include it or not but as long as it’s me - no.
Please review and test https://git.drupalcode.org/project/lms/-/merge_requests/111/diffs?commit... - optimized a bit. If all fine, we can merge this.
Almost there ;)
Unfortunately als experienced this with the kickstarter repo config and also noticed that even without any changes the view config is missing things (for example try to edit the table display settings, save without any changes and export the view). I updated the view config in the kickstarter repo and in the lms_classes/config/install, hope that helps.
Not able to fix all cases of wrong config so I'm afraid it may be needed to fix the view manually after updating to 1.1.x in some cases.
I'll leave this as active so maybe there are any ideas and link the kickstarter repo config for the view: https://github.com/graber-1/drupal_lms_ddev/blob/dev/config/sync/views.v....
Just one more thing, either a regression or a pre-existing issue with the full text plugin. Should be fixed in the trait if possible so if validation fails when checking for feedback, it's visible on the screen regardless of the plugin used. More info on the MR.
Added a stub module to 1.0.x, there'll be an additional step in the upgrade - enabling it. Can't seem to do it programatically, seems like \Drupal::service('module_installer')->install(['lms_classes']); is not enough..
1 BC issue though: I couldn't figure out how to install the submodule in an update hook (anything results in missing lms_classes plugin) or even using Drush (unable to install due to existing config). Update path will be possible but tricky if there are no good ideas / existing examples, probably manually deleting the existing config and installing the module afterwards.
Tested this a bit.. without classes we have a very "vanilla" LMS where students / results view needs to be built.. Everything needed to do this (course status relationship, status and results link fields) is provided.
Will probably need additional docs in the lms_classes module and definitely a new minor bump release for lms_membership_request when this lands.
Feedback in the MR :)
All green.
Needs human review & hopefully a bit of testing.
Pushed some WIP.
When eventually render objects will be actually rendered, the only way to have the unprocessed form will be deep cloning of the form object so 12899++.
Instead of doing what's been done in
📌
Slowly, very slowly start OOPifying the render system
Needs review
, I'd start from supporting rendering of those objects in some ObjectRenderer that would be used in the current renderer conditionally if an object is passed to the render() method - creating new API next to the existing one at the very bottom of the system and only then switching gradually and not implementing some mixed array/object things in the middle of the processing chain so also ++ for reverting
📌
Slowly, very slowly start OOPifying the render system
Needs review
and rethinking the entire concept.
Looks good.
This will be more tricky as it'll break
lms_membership_request →
module.
lms_membership_request →
will now need 2 cases: lms_classes installed or not.
We should probably just do it without looking at lms_membership_request and update it when someone else decides to support the initiative and then update it?
I can do the work here as it doesn't look very scary but the membership request part may be a different thing.
Let's see if this will not affect automation..
Maybe I was a bit too fast with planning of moving the classes sub-module outside of the Drupal LMS repo as it can go out of sync at some point and will require splitting automated tests..
On the other hand.. Students don't have to be members of a course really to be able to take it with group permissions. I'll add some implementation thoughts to 📌 Considering factoring out classes to an optional module Active .
Ahh and class names can be 100% configurable as patterns with placeholder replacement (predefined or using tokens). Not a big task really.
From my knowing so far no one from the corporate world with "senior managers" on board reached out to the project maintainers or developers involved to help with their e-learning projects based on Drupal LMS. If they do, I'm sure we can work more on the project and accommodate everyone's needs.
Classes (whatever they are called) serve the purpose of separating students (whatever called) from teachers (whatever called) as it takes place in the real world education. The more a model differs from the real world, the less flexible it is and the more limitations it creates.
If needed, the classes UI can be hidden completely on a specific project and changing some translatable labels was never a big problem.
When @nicxvan mentioned ‘initializeInternalStorage’ on Slack I thought that we could save an unprocessed version that wouldn’t be a reference then. Something like ‘$this->unprocessed = $element’ (no reference) and use that when getting the unprocessed form.
Merged but this will be released in a new minor beta so it gets sufficient testing and feedback from the community before being marked as stable.
Implemented dereferencing of children in the copy MR, this fixes LMS issues.
One question: since it wasn't about the `##object` values of each element but about certain form elements being references and therefore altered as memory addresses at their source by other code.. don't we actually have a different root cause of now altering more than we altered before?
The solution works also when removed the line that unsets `$copy['##object']`: https://git.drupalcode.org/project/drupal/-/merge_requests/12901/diffs?c...
so that being there wasn't the issue.
3539320-element-plugin-object branch - fixes the LMS issue
3539320-copy branch - empty string textfield `#value` is still cached.
Ok, it doesn't solve the issue in all cases because there are other situations where a modal form should get cached before final submit (failed validation, AJAX widgets like adding field items or file uploads, ..).
I don't know if it'll solve the issue completely for LMS but not caching the form on initial build is going to be a performance improvement in any case. I'll try it now.
So far found out that for some reason on AJAX call the resulting textfield element '#value' is set to an empty string after the change which prevents FormBuilder::handleInputElement() to set its value from user input.
Makes sense, updated.
I'd use \array_key_exists() for a more strict check but this looks good in any case, thanks!
Moving to RTBC and updating priority to critical as it results in fatal errors on sites, emails not boing sent and potentially breaking any logic that could be implemented after rendering of the messages.
Thank you, I'm too busy with all those projects recently to handle all that, added you as a maintainer.
@catch lessons of a started course respect same rules when revisions are enabled, it's just the entity loading logic is different:
course_status -> lesson status (if exists) -> lesson revision reference
For the revisioning UI, what if the edit forms have radio buttons, with no default selection, instead of a revision checkbox?
It's one extra click in the UI vs a conscious choice. I have no idea really, we need to get more opinions on this, preferably something for later as it's something very easy to alter by project anyway.
Can we not delete all comments that explain what's going on in the code?
Also I see interface method docs are replaced by @inheritdoc. Since it's an interface that defines those methods, what is it that we inherit from?
Thanks, that clarified a bit.
I think we're good :)
Added a draft MR, all seems to be working but still a lot of testing is needed and nav block cacheability needs to be updated.
One thing is very annoying: we need an update hook that converts the old field types to the new one, copies target_id data from the old tables and sets new data basing on current entity revisions. We can also make this feature available in the next major but still - it wouldn't be nice if we didn't provide an upgrade path.
I'm not any good with that, spend a lot of time on it already with highly unsatisfying results so.. If anyone wants to help or provide a good example from which I could just copy-paste some code, we can do it. Been there: https://www.drupal.org/docs/drupal-apis/update-api/updating-entities-and... → , https://www.drupal.org/project/field_type_converter →
If not - let's release that next major ASAP while we still don't have any or very little projects in production phase. Maintaining 2 branches will also be too much pain.
Thoughts very welcome.
Thank you for reporting that @buzzbee, the upcoming work on 🌱 Proposal for course revisioning strategy Active Will solve that as a byproduct.
Ok. There is a views dependency in the .info file actually.
Ahh, correct, I'll move it. Somehow thought we have that dependency but it's installed only in automated tests and obviously in the kickstarter repo.
Hiding the previous patch, pls use the MR now.
Merged this after making some changes:
Dropped custom fields UI and renamed to `extra_fields`. Those can now be added by the new hook
Dropped the second module handler invoke as that can be done in a theme preprocess function
Constructor property promotion in the new Views row plugin
Removed duplicated custom fields code from component twig
Please create follow-ups if needed, I think we'll need a bit of automated test coverage for that, in short visit the view page after some course tests are done and check if start link text is as expected and if other fields contain correct values in a foreach loop.
Left some more comments on the MR, let's strip this as much as possible from extra fields and config in favor of exposing API if needed. Looks great otherwise.
This will also affect caching a lot - nav block will depend on course status lessons and not course lessons. Course nav structure cache will have to be dropped.
This will be bigger as we'll have to reference lesson revisions from course status as we currently reference activities from lesson status. The good thing is consistency though. That should be the first step of the work here. The first step alone will only introduce needless data storage though.
May need some additional cleanup and includes a bit more than just adding the form mode (restructured the module slightly not to override the core media form class, also removed delete mode - can be restored if needed).