Account created on 24 October 2011, over 13 years ago
#

Merge Requests

More

Recent comments

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

Thank you for reporting that @buzzbee, the upcoming work on 🌱 Proposal for course revisioning strategy Active Will solve that as a byproduct.

🇵🇱Poland Graber

Ok. There is a views dependency in the .info file actually.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

Hiding the previous patch, pls use the MR now.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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).

🇵🇱Poland Graber

The current steps to reproduce the issue are not sufficient, tried to reproduce on gin with between filter and got no warnings.

🇵🇱Poland Graber

graber made their first commit to this issue’s fork.

🇵🇱Poland Graber

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

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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
🇵🇱Poland Graber

Finally. Those local - remote test differences will kill me one day.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

Merged and created one follow-up to optimize this a bit ( 📌 Course nav - additional caching optimization Active ).

🇵🇱Poland Graber

Cache is enabled.
All this will need extending automated tests coverage but I'll get to that in a separate issue.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

I think going back to #37, forgetting about everything after and resuming from there may be a good idea ;)

🇵🇱Poland Graber

Merged, let's have follow-ups if needed.

🇵🇱Poland Graber

graber changed the visibility of the branch 3515744-convert-course-navigation to hidden.

🇵🇱Poland Graber

Added an explanation and merged.

Thanks for checking!

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

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).

🇵🇱Poland Graber

NW over a space? 😂
Can't believe..
Just merge the suggestion and continue reviewing!

🇵🇱Poland Graber

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!

🇵🇱Poland Graber

No worries, just merged 📌 Improve / fix course navigation logic Active , let's use training manager API as much as possible here.

🇵🇱Poland Graber

Merged. Pls open any follow-ups if needed.

🇵🇱Poland Graber

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.

🇵🇱Poland Graber

graber changed the visibility of the branch 0.2.x to hidden.

🇵🇱Poland Graber

graber changed the visibility of the branch 3467618-h5p-unfork to hidden.

🇵🇱Poland Graber

graber changed the visibility of the branch 3467618-allowed-attempts to hidden.

🇵🇱Poland Graber

graber made their first commit to this issue’s fork.

🇵🇱Poland Graber

All remarks in the MR diff.
Great work and not far away from making this work 100%!

🇵🇱Poland Graber

Yes, an alter hook invoked from __construct() with cached output should be fine. We'll need to think where to cache it though (or maybe don't cache it?)

🇵🇱Poland Graber

Hmm, the situation is exactly the same with A360 Xapi activities. The only way to alter the statements would be using a proxy with an alter option but that's an additional middle step which means performance drop.

Production build 0.71.5 2024