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

Merge Requests

More

Recent comments

🇵🇱Poland Graber

I decided to merge your PR as I know from my past experience it’s the best way to get some feedback from others and we should follow that kind of policy in the future.
Thank you for your input!

Let’s handle other plugins in child issues, leaving this one as “needs work” so we have a place for general considerations.

🇵🇱Poland Graber

Regarding your MR, All standards corrected but I think that if there's the "check feedback" button, when going back to an answered activity the feedback shouldn't show.
Somehow I still like the no-button version with immediate feedback more so I guess we need a third opinion here ;)

🇵🇱Poland Graber

In my opinion, the feedback and non-feedback plugins are mutually exclusive -- any course will want to use 100% one or the other. A course that only gives a grade at the end will not want feedback along the way, and vice versa. So we could have a radio-select course setting with "Course Type" options: Standard (grade at end) or Interactive (feedback throughout).

We can't be 100% sure of that, those are plugins and developers can make any types of them plus site builders can create unlimited combinations of activity field setups and plugins. You can go for a custom solution here and limit available activity types based on some course checkbox.

Rather than having separate plugins for feedback and non-feedback versions of each question type, could we use a strategy class that's set by the course's feedback setting? Then the feedback data storage, editing, and display would only be injected when called for by the strategy class, all in the same plugin as the standard function.

No, because feedback storage depends on actual plugin and can differ a lot, there's no single, ultimate storage that will fit all.

If we extend the LmsAnswer field type and widget, won't that keep feedback data in the schema for non-feedback questions? Instead, could we have a dedicated feedback field that's only added when called for by the strategy class? That would also give us a lot more flexibility for improved display and course-creator UX.

I thought about extending in a separate field plugin, specific to the new multiple select with feedback activity-answer plugin only.

Logistically, I'm still thinking of building all of the feedback plugins separately first.

Plugin-by-plugin development approach is the best way here indeed as those plugins may not have much in common in various cases really and data storage is handled by each plugin individually.

🇵🇱Poland Graber

Agreed, the LmsAnswer field type and widget will need to be extended for that one.

As for the T/F MR by @arvana: https://git.drupalcode.org/issue/lms-3502281/-/jobs/4339487

Let's create child issues for next plugins instead of handling here.

Also thinking how to separate feedback plugins from non-feedback ones but no idea at the moment, because both may use the same base classes so separate modules are not an option.. but maybe if we moved base classes' contents to traits and placed them in the LMS module? Thoughts anyone?

🇵🇱Poland Graber

No, it's safe to use, I just don't have time to support 2 versions in the issue queues and already doing more than enough charity work ;)

🇵🇱Poland Graber

Updated, including tests a bit as well to illustrate direct usage of

return $response->mergeWith($other);
🇵🇱Poland Graber

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

🇵🇱Poland Graber

graber changed the visibility of the branch 3343670-allow-to-merge-ajaxcommands-11-x to hidden.

🇵🇱Poland Graber

Nice one, one nit, it'll need some testing then.

🇵🇱Poland Graber

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

🇵🇱Poland Graber

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

🇵🇱Poland Graber

@borisson_, don't we need config schema for this new setting?

🇵🇱Poland Graber

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

🇵🇱Poland Graber

Parameter no longer used.

🇵🇱Poland Graber

API change.

🇵🇱Poland Graber

API changed, you need to do the same as in Drupal\actions_permissions\EventSubscriber\ActionsPermissionsEventSubscriber::alterActions() now (actions_permissions is in views_bulk_operations/modules).
Docs need updating.

🇵🇱Poland Graber

Yes, for your #3 - we’ll need to support course bundles so they can have different settings. The API is flexible enough but it means other modules like lms_membership_request (actually can’t think of any other that would be affected at the moment) will have to adapt. I’ll leave that for sometime later.

🇵🇱Poland Graber

Just ran into it on 3.3.3 with grequest module.
For me it happened when I had some field storage config that is normally installed by a relationship plugin already installed, reinstalling the plugin caused this error.
Probably some existing config check missing, deleting that storage config resolved the issue.

🇵🇱Poland Graber

Adjusted a bit and merged.

🇵🇱Poland Graber

Logo by @arvana, already added, thank you!

🇵🇱Poland Graber

@arvana I had to adapt the API a bit so we have form state available in activity-answer plugin answeringForm method for this to work fine.
Now you have an example True/False with feedback plugin: https://git.drupalcode.org/project/lms/-/blob/1.0.x/modules/lms_answer_p...

🇵🇱Poland Graber

Looks good!
Still some minor things to improve though.

🇵🇱Poland Graber

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

🇵🇱Poland Graber

@catch, we can still implement some constraints - these will prevent edition of an entity. So far I prevented deletion and added some warnings on edition attempts. Pls check when you'll have some time.
I may add some automated tests later.

🇵🇱Poland Graber

Looks good to me, marking as RTBC, thanks!

🇵🇱Poland Graber

Left some comments in the MR.

🇵🇱Poland Graber

Of course, on a specific platform you can have only questions with feedback and automatically evaluated configured if you want no teacher interaction, no grading etc. Course creators will then not be able to create any learn-grade-pass course workflows but only non-graded where scores will serve some other purpose.

🇵🇱Poland Graber

Do you see a separate set of plugins that incorporate immediate answer checking and feedback, which would eventually be a duplicate set of all the non-feedback plugins? Or should each existing and future plugin include both options of giving feedback or not? It seems like the second option would be more efficient overall.

I don't think the second option would be more efficient because it requires additional storage that would hold empty columns (or even empty data tables) in case when feedback will be off. Also, some plugins (the teacher - evaluated ones like fulltext answer) just don't allow any immediate feedback so will not have their feedback counterparts.

A more architectural question is whether each question in a course should offer the individual option of giving immediate feedback, of if an entire course needs to be one way or the other.

- by question type - if it has a plugin that gives immediate feedback - feedback will be provided.

We should probably start with a single plugin that gives feedback (True/False as it's the most simple one?) and see how it works.

🇵🇱Poland Graber

This is actually specific to an activity-answer plugin, some of those (basically all automatically scored), can provide feedback to all possible answers or answer ranges (for example if there was a math question what's 2+2 and a number input field, feedback may depend on ranges of the given answer and if it's a multiple choice, feedback should be bound to all possible answers and displayed depending on user choice with different possible scenarios, free text plugins could recognize some patterns and provide feedback basing on that).

In the case of radios, we don't even need an additional button as feedback can be displayed with AJAX when a choice is made, in case of a multiple choice, there should possibly be a "check" button that is replaced by next/corrctt answer.

Due to the above, I don't see a possibility for a general API or even generic feedback storage, all depends on the actual plugin behavior and requirements.

So: try to start creating immediate feedback activity-answer plugins (a few basic ones could even go to another sub-module in the LMS core similar to lms_answer_plugins and more complex ones to ecosystem modules, we can decide case-by-case), if the LMS core API needs to be extended further for this purpose, please add feedback here.

🇵🇱Poland Graber

This will not break any contrib modules implementing this hook and still returning void, because we don't have a type hint and it is still a hook so even if we had a type hint in file.api.php file, hook implementations wouldn't necessarily add those and we don't check the return type after invoking this hook.
However, we need a change record so contrib module maintainers will now return NULL in their implementations for consistency.

🇵🇱Poland Graber

I'd go for absolutely minimalist approach here, without even adding any css, just convert those templates to something more simple or even completely drop them and for example use Core #item_list in place of lms_lp_step_module_activity.
As little and as clean as possible here and any work in an ecosystem theme to make this awesome.

🇵🇱Poland Graber

graber changed the visibility of the branch drupal-3501398-3501398-missing-null-in to hidden.

🇵🇱Poland Graber

Ok, actually see file_file_download(), it returns void in all the cases instead of NULL. I think that should be included in the doc comment.

🇵🇱Poland Graber
  1. Where does the function actually return int?
  2. It doesn't return anything if the if condition is not met (void).

I think we should add a :?array return type hint and actually return NULL if the if condition is not met.

🇵🇱Poland Graber

Addressed all threads, thanks for reviewing!

🇵🇱Poland Graber

Setting this to needs review.
Follow-ups to create:

  1. Improve Drupal\lms\Plugin\ModalSubform\EntityForm::access()
  2. Scrolling to elements after AJAXing
  3. Views exposed form submit on the LMS entity selection views shouldn't be in the modal buttons area
  4. Next minor phpstan and phpunit
  5. Extend test coverage with creation of new entities and modal on top of a modal (creating a lesson and its activities)
🇵🇱Poland Graber

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

🇵🇱Poland Graber

Needs an event subscriber similar to the one in lms_xapi_lesson: https://git.drupalcode.org/project/lms_xapi/-/blob/1.0.x/modules/lms_xap... since 📌 Allow more flexible identifiers for LRS entries Active landed.. or we can switch to tagged services and use that instead..

Or we can make all that follow-ups, just switch back to review then and I'll merge.

🇵🇱Poland Graber

Let's have a follow up with tagged services as I need to move to other things now. Fixed some bugs and merged.

🇵🇱Poland Graber

I see the documentation got updated and the issue description stayed the same.

🇵🇱Poland Graber

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

🇵🇱Poland Graber

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

🇵🇱Poland Graber

Down to 17, worth checking some of those eventually but phpstan also has issues plus fixing some would be needless struggle only.
Merge train running.

🇵🇱Poland Graber

Also not sure about the complexity of installing config that is not config yet.. worth investigating at some point when we'll have time for that.

🇵🇱Poland Graber

Let's use this as a plan, we'll need cspell, phpcs, phpstan and test coverage. And probably a few other issues resolved.

🇵🇱Poland Graber

Commented on that core issue with a link here and linked it in this module description. Just please don't comment there anymore, d.o. barely keeps up with such long ones, saving my comment took more than a minute.

🇵🇱Poland Graber

Big issues like this one tend to (almost) never get solved due to various blockers in core gates, plus infra limitations (this is an epic really that should have sub-issues). One good example is even longer entity reference filter issue that got closed and continued in a new issue due to so many comments that d.o. infra couldn't keep up.
Maybe it'll be more sensible to continue work on the Views Date Filter project and move it to core when ready?

🇵🇱Poland Graber

There's this one.. but I remember other issues this module solves, I don't have more time to search now though.

🇵🇱Poland Graber

Ahh, I thought it’s about changing views config within a workspace. That’s a different story 🙂

Production build 0.71.5 2024