Problem/Motivation

I think there will be sites that don't want to use classes, so we should consider factoring it out to an optional module.

Sites without it could use a 'student' role, or no role at all, in courses with direct memberships.

Steps to reproduce

Proposed resolution

  1. Create a "take course" group permission
  2. Change group "take" access logic to use the above permission
  3. Modify ClassPermissionCalculator to grant the above permission on all parent courses to all class members
  4. Modify the students view to show all Course Status entities from the given course
  5. Move all class-related logic to a LMS sub-module (modules/lms_classes)
  6. Create a second students view for lms_classes (copy the current students view) that shows students in classes
  7. (to be considered later) Deprecate lms_classes to remove in the next major and on release copy the submodule to new contrib "lms_classes" (https://www.drupal.org/project/lms_classes)

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork lms-3540096

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

graber’s picture

graber commented
Issue summary: View changes
graber’s picture

graber commented
Title: Considering factoring out classes to an optional module » Move classes to an optional module
Issue summary: View changes

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

catch’s picture

catch
he/him
Primary language English
commented

Ah yes I was thinking just an optional module in the main repo - at least to start with.

graber’s picture

graber commented

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.

catch’s picture

catch
he/him
Primary language English
commented

Could lms_membership_request add a hard dependency on lms_classes?

I would think the non-class case would be pretty much raw https://www.drupal.org/project/grequest or at least not far off? But if there's some extra things we want to add, that becomes a feature request against lms_membership_request rather than a bug then.

graber opened merge request !112

graber’s picture

graber commented
Assigned: Unassigned » graber

Pushed some WIP.

graber’s picture

graber commented
Assigned: graber » Unassigned
Status: Active » Needs review

All green.

Needs human review & hopefully a bit of testing.

graber’s picture

graber commented

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.

graber’s picture

graber commented

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 or adding an empty lms_classes submodule to 1.0.x and enabling it before update.

graber opened merge request !114

catch’s picture

catch
he/him
Primary language English
commented

(unable to install due to existing config)

Can the config be moved to optional? That ought to not complain.

graber’s picture

graber commented

Added a stub module to 1.0.x, there'll be an additional step in the upgrade - enabling it. Can't seem to do it programatically, seems like \Drupal::service('module_installer')->install(['lms_classes']); is not enough because executing drush en lms_classes afterwards results in actually installing of the module..
Also tried moving config to optional but it still doesn't solve the "The "lms_classes" plugin does not exist" error on drush updb. The current solution with an extra lms_classes installation step before upgrading is the best I could come up with. Going to merge this sometime soon since there's no feedback/reviews, we can always create follow-ups if needed.

  • graber committed 5e4778e5 on 1.1.x 
    Issue #3540096: Move classes to an optional module

  • graber committed d09ddf11 on 1.0.x 
    Issue #3540096: Move classes to an optional module
graber’s picture

graber commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.