Moodle
  1. Moodle
  2. MDL-13803

Old module display script mod.php (which is core) does not support role overrides page for non-formslib modules

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9.1
    • Component/s: Course
    • Labels:
      None
    • Environment:
      Any
    • Database:
      Any
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      30471

      Description

      I have recently done an upgrade of the Questionnaire module to support Roles and Permissions. This does not use formslib, so is accessed through mod.php rather than modedit.php. However, the roles overrides tab was not showing. Comparing it to modedit, the problem appeared to be the omission of these two lines from mod.php.

      if (!empty($cm->id))

      { $context = get_context_instance(CONTEXT_MODULE, $cm->id); $currenttab = 'update'; //Added lines - start $overridableroles = get_overridable_roles($context); $assignableroles = get_assignable_roles($context); //Added lines - end include_once($CFG->dirroot.'/'.$CFG->admin.'/roles/tabs.php'); }

      I've included a patch for this module, built against 1.9. It should also correct the problem if it is happening on HEAD, because course/mod.php appears to be identical between 1.9 and HEAD, I will shortly be installing a HEAD environment anyway and will test it then.

      (I suppose there is just a small risk that this could cause problems with other modules which may have been written to be reliant on it not working properly).

        Activity

        Hide
        Tim Hunt added a comment -

        We discussed this on Moodle HQ chat yesterday, and this should go into 1.9 and HEAD. Probably 1.8 too, if it is needed there.

        Just go for it Gareth.

        Show
        Tim Hunt added a comment - We discussed this on Moodle HQ chat yesterday, and this should go into 1.9 and HEAD. Probably 1.8 too, if it is needed there. Just go for it Gareth.
        Hide
        Petr Škoda added a comment -

        please test and commit 1.8 – HEAD
        thanks

        Show
        Petr Škoda added a comment - please test and commit 1.8 – HEAD thanks
        Hide
        Gareth Morgan added a comment -

        I've tested this, adding and editing a module that is not roles/capabilities compliant and does not use formslib (module "dialogue").

        The result was not quite what I wanted - it still displayed Locally Assigned Roles and Override permissions tabs. The Locally Assigned Roles showed the standard default roles, and probably should be visible here.

        The Override permissions tab only had one permission: moodle/site:accessallgroups which is presumably something that can be overriden for all modules.

        I'm conscious that my change will mean that these tabs will now appear for a number of modules where they previously did not. Having said that, there are not many modules that do not use formslib now.

        Shall I commit anyway, or will it be necessary to do something to suppress display of one or both tabs when there are no capabilities local to the module?

        Show
        Gareth Morgan added a comment - I've tested this, adding and editing a module that is not roles/capabilities compliant and does not use formslib (module "dialogue"). The result was not quite what I wanted - it still displayed Locally Assigned Roles and Override permissions tabs. The Locally Assigned Roles showed the standard default roles, and probably should be visible here. The Override permissions tab only had one permission: moodle/site:accessallgroups which is presumably something that can be overriden for all modules. I'm conscious that my change will mean that these tabs will now appear for a number of modules where they previously did not. Having said that, there are not many modules that do not use formslib now. Shall I commit anyway, or will it be necessary to do something to suppress display of one or both tabs when there are no capabilities local to the module?
        Hide
        Eloy Lafuente (stronk7) added a comment -

        While at a fist thought it could have sense to prevent the visualization of the overrides if the module hasn't capabilities... I really think it's better to show it always... "forcing" developers to think in a capabilities-world.

        So my +1 to commit without "conditionally" showing/hiding any tab.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - While at a fist thought it could have sense to prevent the visualization of the overrides if the module hasn't capabilities... I really think it's better to show it always... "forcing" developers to think in a capabilities-world. So my +1 to commit without "conditionally" showing/hiding any tab. Ciao
        Hide
        Gareth Morgan added a comment -

        I've tested and checked in fixes to branch MOODLE_19_STABLE and to HEAD.

        MOODLE_18_STABLE (1.19.2.4 in CVS) does not appear to need this fix at present because it unconditionally forces a "Roles" tab to be visible, from which assignments and overrides are accessible (Martin took this out of the 1.9 branch and HEAD). It is probably not the right way to do this, but it looks as if a decision has apparently been made to leave it in.

        Show
        Gareth Morgan added a comment - I've tested and checked in fixes to branch MOODLE_19_STABLE and to HEAD. MOODLE_18_STABLE (1.19.2.4 in CVS) does not appear to need this fix at present because it unconditionally forces a "Roles" tab to be visible, from which assignments and overrides are accessible (Martin took this out of the 1.9 branch and HEAD). It is probably not the right way to do this, but it looks as if a decision has apparently been made to leave it in.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Should we close this, then? As implemented for 1.9.1 and 2.0 ?

        Show
        Eloy Lafuente (stronk7) added a comment - Should we close this, then? As implemented for 1.9.1 and 2.0 ?
        Hide
        Gareth Morgan added a comment -

        Sorry for the delay in closing this bug. Marked as fixed against 1.9.1 (although it is also fixed on HEAD) because according to people I've spoken to, if something is checked in to MOODLE_19_STABLE, it is assumed that it will be merged into HEAD anyway, so does not need to be marked as fix version 2.0.

        Show
        Gareth Morgan added a comment - Sorry for the delay in closing this bug. Marked as fixed against 1.9.1 (although it is also fixed on HEAD) because according to people I've spoken to, if something is checked in to MOODLE_19_STABLE, it is assumed that it will be merged into HEAD anyway, so does not need to be marked as fix version 2.0.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: