Problem/Motivation

The activity form shows 'Assign a skill to this activity', even when the skills system is not enabled.

Proposed resolution

Attached patch is a quick fix that adds the missing moduleExists() check.

However, it would be better for opigno_skills_system to implement hook_form_FORM_ID_alter() to add these elements to the activity form itself - this would save having to check if the module exists and make the logic more independent.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 opigno_module-3075173-4.patch651 bytescatch
#3 opigno_module-3075173-3.patch651 bytescatch

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new opigno_module-3075173-3.patch651 bytes

Here's a patch, although as mentioned the check could be saved altogether by moving the logic into a form alter in opigno_skills module. Could also go further and make the skills widget an 'extra field'.

axelm’s picture

axelm
Primary language English
commented
Status: Needs review » Closed (works as designed)

Hi @catch
Opigno was designed like that.
The option "Activate skills system for this module" makes possible to have modules automatically ending when reaching a target skill.
But if this option is not enabled, you can anyway assign skills to activities so that you have a reporting on the users' skills.

You can have a look at our user manual to learn more about Opigno skills system:

https://opigno.atlassian.net/wiki/spaces/OUM20/pages/790167560/Automatic...

catch’s picture

catch
he/him
Primary language English
commented
Status: Closed (works as designed) » Needs review
StatusFileSize
new opigno_module-3075173-4.patch651 bytes

Fixing the variable name, although that should also use an underscore instead of camel case to match Drupal coding standards.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented

Cross-posted the last two comments.

@axelm further up in the same form there's a check for the opigno_skills_system module though, are you saying the skills system isn't optional at all?

axelm’s picture

axelm
Primary language English
commented

@catch No this is no optional

catch’s picture

catch
he/him
Primary language English
commented

@axelm if the skills system isn't optional, then why is the module itself optional?

axelm’s picture

axelm
Primary language English
commented

@catch
Skills system (= the possibility to assign skills to activities and then have a reporting) is part of Opigno.

The option in modules is to have Opigno automatically managing when the module will end (=you define a target skill and Opigno will continue presenting specifically targeted activities to the user until the target skill is mastered). If this option is not enabled you defined which activities will be presented to the users taking this module and the module will end with the last activity.

This is explained in our user manual
https://opigno.atlassian.net/wiki/spaces/OUM20/pages/790167560/Automatic...

imclean’s picture

imclean commented

#8:

No this is no optional

#10

Skills system (= the possibility to assign skills to activities and then have a reporting) is part of Opigno.

It seems it's more a requirement than a possibility. Is there any chance the skills feature can be made optional? We have no need for the complexity of skills in one of our projects.

In this case, the following in Drupal\opigno_module\Form\OpignoActivityForm->buildForm() is causing an error when no skills are assigned:

    // Get list of skills trees.
    $target_skills = $term_storage->loadTree('skills', 0, 1);
    $default_target_skill = $target_skills[0]->tid;
    $options = [];

The error is:

Notice: Trying to get property 'tid' of non-object in Drupal\opigno_module\Form\OpignoActivityForm->buildForm() (line 104 of modules/contrib/opigno_module/src/Form/OpignoActivityForm.php).

imclean’s picture

imclean commented

I'm seeing 2 other errors.

Notice: Undefined offset: 0 in Drupal\opigno_module\Form\OpignoActivityForm->buildForm() (line 61 of modules/contrib/opigno_module/src/Form/OpignoActivityForm.php).

.

This occurs because there is no value for auto_skills.

    // Hide field 'auto_skills' for all existing activities.
    if (!$is_new || !$moduleHandler->moduleExists('opigno_skills_system') || (isset($module) && !$in_skills_system)) {
      $form['auto_skills']['#access'] = FALSE;
      $auto_skills = $this->getEntity()->get('auto_skills')->getValue()[0]['value'];
    }
Notice: Undefined offset: 0 in Drupal\opigno_module\Form\OpignoActivityForm->buildForm() (line 104 of modules/contrib/opigno_module/src/Form/OpignoActivityForm.php).

Which relates to the same line as the error in #11.

imclean’s picture

imclean commented

Uninstalling Opigno Skills System resolved the errors. Apologies for the noise. The errors remain.

catch’s picture

catch
he/him
Primary language English
commented

Opened an issue to fix the notice mentioned in #11, but agreed it would be better if the entire opigno skills system could be made optional rather than just the auto-skills feature.

#3085194: Activity form PHP notices.