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).
The current steps to reproduce the issue are not sufficient, tried to reproduce on gin with between filter and got no warnings.
do you mean we should create a new ecosystem module for lms_answer_plugins?
No. Ok to have it in lms/modules/lms_answer_plugins
Looks good!
As an alternative linking core issue to create centralized dialog form API that for example allows to add / edit entities (even with 2 levels of nesting) in Drupal LMS using forms in modal dialogs and rebuilding the parent forms:
✨
Create a dialog subform API
Active
- the solution is there in Drupal LMS repo but needs more generic test coverage, cleanup and documentation.
Adding some initial thoughts on this:
- New Activity-Answer plugin in the lms_answer_plugins submodule
- Editing: Text field with some placeholders (_answer1_, _answer2_, ..)
- Editing: Multiple string field with possible answers to fill, if empty, test input will be required from the student, if not empty - validation if the number of placeholders is the same as the number of values
- Answering: if possible answers field is empty - text input elements in place of placeholders, otherwise single-select with AJAX to reduce number of options once selected
- Score calculation - max number of points equal to total number of blanks, +1 point for a correct selection, 0 for incorrect, final score equal to acquired points / max points.
- Follow-up: add drag n drop functionality on js side
- Follow-up: add immediate feedback option with a feedback column in the possible answers field (as correct and incorrect) - to refine
Finally. Those local - remote test differences will kill me one day.
Actually.. maybe it’s not going to be so bad as we’ll just need to add a new column to an existing schema, reference field names will not change after all.
Just a “vid” field will not be enough, for example lesson status references multiple activities, that’d be too messy. I also wouldn’t like to add an additional dependency but we can use some of the Entity Reference Revisions code as a reference - a new field type plugin should be enough in our case.
Update may be a bit tricky.
1. Latest changes from 1.0.x need to be included
2. Some coding standards tests fail
3. We should avoid having a hard dependency on media module.
We could make a global setting so the site owner can choose if students take the course / lesson versions from the moment of starting or current, that'd also solve any possible BC issues.
Merged and created one follow-up to optimize this a bit ( 📌 Course nav - additional caching optimization Active ).
🫥
Cache is enabled.
All this will need extending automated tests coverage but I'll get to that in a separate issue.
Yes, just.. let's not treat this as VBO-only issue. The context provider should be something like "Group from the current View arguments", otherwise one day we may end up with a similar issue but for a different module that calls View access() method.
I think going back to #37, forgetting about everything after and resuming from there may be a good idea ;)
Merged, let's have follow-ups if needed.
graber → changed the visibility of the branch 3515744-convert-course-navigation to hidden.
graber → created an issue.
graber → made their first commit to this issue’s fork.
Added an explanation and merged.
Thanks for checking!
Just that I remember some time before @var and @param tags were required by our coding standards and it's no longer the case. Probably because of type hints that make them obsolete in a majority of cases.