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.
All threads resolved. Just a small note to @smustgrave: @param doc comments are not needed if parameters are well described by their names and type hints and that was the case here.
Setting to RTBC.
Thanks, created 📌 Extend course navigation test coverage Active as a follow-up.
if you can revisit after a course is graded, I think you would be able to revisit before?
We don't know what different requirements will we encounter in the future.
I'd expect this or that client will not want people to revisit until fully graded (let's say it's an exam with some open questions - editing answers after finishing and before everything is graded would defeat the purpose).
NW over a space? 😂
Can't believe..
Just merge the suggestion and continue reviewing!
Merged.
One thing I'd change is use int class constants for those options instead of string values but that's not a blocker. Thanks!
No worries, just merged 📌 Improve / fix course navigation logic Active , let's use training manager API as much as possible here.
Merged. Pls open any follow-ups if needed.
Just fixed a resulting issue locally in a EmailAdjuster plugin.. the scenario is that Webform uses tokens in its addresses and sometimes there can be a comma in entity label / field. An email is inited with Drupal\symfony_mailer\Plugin\EmailBuilder\LegacyEmailBuilder setting from_name with a comma and that gets mapped to "from" address in webform_mail. Not even 100% sure where this should be solved but probably here or in a follow-up as those commas will definitely not going to be ever escaped anywhere.
I think comma-separated strings should never be treated as multiple addresses and an array should always be used for strict behavior.
Also adding a patch for 3.0.3.
graber → created an issue.