Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40457

Question Bank - Allow plugins to add columns to question bank view

    Details

    • Testing Instructions:
      Hide

      Attached is a local plugin which uses this API to display the last-modified-date of questions. Install the plugin.

      1. Go to Course admin -> Question bank
      2. Ensure there are some visible questions in the bank.
      3. Verify there is not a column showing the last modified date in the Question Bank display

      4. In config.php, add: $CFG->questionbankcolumns = 'checkbox_column,question_type_column,question_name_column,edit_action_column,preview_action_column,copy_action_column,delete_action_column,creator_name_column,modifier_name_column,local_questionbankmodified_question_bank_column';

      5. Verify there is now a column showing the last modified date in the Question Bank display.

      6. Create or find a quiz
      7. Go to its Edit quiz page
      8. Verify you can see the question bank there and no regressions.
      9. Add to config.php $CFG->quizquestionbankcolumns = 'add_to_quiz_action_column,checkbox_column,question_type_column,question_name_column,edit_action_column,preview_action_column,copy_action_column,local_questionbankmodified_question_bank_column';
      10. Ensure the question bank where you can select questions to add now has the last modified date column.

      Show
      Attached is a local plugin which uses this API to display the last-modified-date of questions. Install the plugin. 1. Go to Course admin -> Question bank 2. Ensure there are some visible questions in the bank. 3. Verify there is not a column showing the last modified date in the Question Bank display 4. In config.php, add: $CFG->questionbankcolumns = 'checkbox_column,question_type_column,question_name_column,edit_action_column,preview_action_column,copy_action_column,delete_action_column,creator_name_column,modifier_name_column,local_questionbankmodified_question_bank_column'; 5. Verify there is now a column showing the last modified date in the Question Bank display. 6. Create or find a quiz 7. Go to its Edit quiz page 8. Verify you can see the question bank there and no regressions. 9. Add to config.php $CFG->quizquestionbankcolumns = 'add_to_quiz_action_column,checkbox_column,question_type_column,question_name_column,edit_action_column,preview_action_column,copy_action_column,local_questionbankmodified_question_bank_column'; 10. Ensure the question bank where you can select questions to add now has the last modified date column.
    • Affected Branches:
      MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-40457-question_bank_plugins_columns

      Description

      This short patch allows plugins to add columns to be displayed in the question bank view. Along with the existing information like question name, creator, and type, a plugin could add a column to display any tags associated with the question, for example, or any other information about questions.

      This complements MDL-40313, which allows plugins to add filtering criteria.
      So for example a plugin could display the creation date of the question, and allow the user to filter questions by creation date, or by tags.

        Gliffy Diagrams

        1. columns3.patch
          4 kB
          Ray Morris

          Issue Links

            Activity

            Hide
            raymor Ray Morris added a comment -

            I'm trying to post a couple of patches fairly quickly because Tim may be working extensively on the same files. If there is a documentation page for these classes which needs to be updated or something similar, I'll certainly do that.

            Show
            raymor Ray Morris added a comment - I'm trying to post a couple of patches fairly quickly because Tim may be working extensively on the same files. If there is a documentation page for these classes which needs to be updated or something similar, I'll certainly do that.
            Hide
            timhunt Tim Hunt added a comment -

            I'm afraid I don't like the design of this:

            1. It assumes that each local plugin will add 0 or 1 other column to the question bank.
            2. It does not let any other plugin type change the questionbank display.
            3. It assumes that if the code of the local plugin wants to add a column to the question bank, then that is the right thing to do for all users.
            4. It only changes the stand-alone view of the question bank. There is no way to change the view of the question bank in the quiz.
            5. It assumes the local plugin will define the column type in lib.php. It does not use the new class auto-loading mechanism.
            6. It is a pull model (question bank asks all plugins if they have a column type) but push might be better (plugins inform the question bank if they provide column types).

            This is something I thought about but never implemented when I first implemented the question_bank_view, question_bank_view, ... classes. (Which, incidentally, was before some modern Moodle programming practices like renderers existed.)

            I think the list of columns actually displayed in the question bank (in each different view) is something that should be controlled by settings, rather than fixed in the code (of core plus plugins). I don't know if this should be an admin setting, or a user prefrerence. Perhaps an admin setting for the default column set, then a user preference to let individuals customise the view for their work-load is the optimum we should aim for in the long-term. At any rate, the current wanted_columns implementation is just a placeholder that was meant to be replaced by something better in future.

            Similarly, known_field_types needs to be replaced by something better at some point.

            Looking at my original design, we have
            $this->knowncolumntypes[$col->get_name()] = $col;
            That now seems like a bad idea to me. It means that the unique identifier used to refer to the column type is a string with no meaning. It would be more verbose, but it might just be better to use the class names. Then, if we have $CFG->questionbankcolumns = "question_bank_checkbox_column,question_bank_question_type_column,...,local_mycustomcolumntype_whatever_column", then it would just work, but that is horribly verbose, and makes it hard to change the class names in future, which I just did for the quiz columns. Hmm.

            I know you were aiming to just do a simple patch to solve your immediate problem (add a few custom columns that you are happy to define in a local plug-in), but I think we ought to consider solving this properly.

            Show
            timhunt Tim Hunt added a comment - I'm afraid I don't like the design of this: 1. It assumes that each local plugin will add 0 or 1 other column to the question bank. 2. It does not let any other plugin type change the questionbank display. 3. It assumes that if the code of the local plugin wants to add a column to the question bank, then that is the right thing to do for all users. 4. It only changes the stand-alone view of the question bank. There is no way to change the view of the question bank in the quiz. 5. It assumes the local plugin will define the column type in lib.php. It does not use the new class auto-loading mechanism. 6. It is a pull model (question bank asks all plugins if they have a column type) but push might be better (plugins inform the question bank if they provide column types). This is something I thought about but never implemented when I first implemented the question_bank_view, question_bank_view, ... classes. (Which, incidentally, was before some modern Moodle programming practices like renderers existed.) I think the list of columns actually displayed in the question bank (in each different view) is something that should be controlled by settings, rather than fixed in the code (of core plus plugins). I don't know if this should be an admin setting, or a user prefrerence. Perhaps an admin setting for the default column set, then a user preference to let individuals customise the view for their work-load is the optimum we should aim for in the long-term. At any rate, the current wanted_columns implementation is just a placeholder that was meant to be replaced by something better in future. Similarly, known_field_types needs to be replaced by something better at some point. Looking at my original design, we have $this->knowncolumntypes [$col->get_name()] = $col; That now seems like a bad idea to me. It means that the unique identifier used to refer to the column type is a string with no meaning. It would be more verbose, but it might just be better to use the class names. Then, if we have $CFG->questionbankcolumns = "question_bank_checkbox_column,question_bank_question_type_column,...,local_mycustomcolumntype_whatever_column", then it would just work, but that is horribly verbose, and makes it hard to change the class names in future, which I just did for the quiz columns. Hmm. I know you were aiming to just do a simple patch to solve your immediate problem (add a few custom columns that you are happy to define in a local plug-in), but I think we ought to consider solving this properly.
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, I forgot to click the worfklow buttons.

            Show
            timhunt Tim Hunt added a comment - Sorry, I forgot to click the worfklow buttons.
            Hide
            raymor Ray Morris added a comment - - edited

            What do you have in mind in terms of #6, "It is a pull model (question bank asks all plugins if they have a column type)
            but push might be better (plugins inform the question bank if they provide column types)"? I'm not sure how that might be
            implemented in this context, and it affects everything else mentioned.

            1. It assumes that each local plugin will add 0 or 1 other column to the question bank.

            Good point, and a simple fix, though the form that takes depends on #6.

            2. It does not let any other plugin type change the questionbank display.

            True, could be better. Also true of the entire Moodle plugin system. All of the Moodle plugin functions
            take a plugin type argument, unfortunately. Reports plugins, enroll plugins, auth plugins etc. probably
            won't be adding columns to question bank, so maybe they don't need the ability to? Sny suggestion on something
            you think would be better, perhaps covered under #6?

            3. It assumes that if the code of the local plugin wants to add a column to the question bank, then that is the right
            thing to do for all users.

            I see it this as "recognizes that if a plugin decides to add a column, the plugin is responsible for deciding when
            it should do so". The plugin knows what it's settings are.

            4. It only changes the stand-alone view of the question bank. There is no way to change the view of the question bank
            in the quiz.

            I'm not sure I understand. Does this refer to the function that quiz_question_bank_view re-implements without
            calling parent::, wanted_columns()? If quiz_question_bank_view wants the full functionality of question_bank_view,
            should it not call $parent::wanted_columns() when it overrides that function? If not, can the same four-line patch
            not be copied over?

            5. It assumes the local plugin will define the column type in lib.php. It does not use the new class auto-loading mechanism.

            Okay. I've read the new http://docs.moodle.org/dev/Automatic_class_loading , implementation again depends on #6.

            6. It is a pull model (question bank asks all plugins if they have a column type) but push might be better
            (plugins inform the question bank if they provide column types).

            What do you have in mind for this?

            > It means that the unique identifier used to refer to the column type is a string with no meaning. ...
            > to use the class names ... is horribly verbose, and makes it hard to change the class names in future,
            > which I just did for the quiz columns. Hmm.

            I strongly suspect you got it right the first time. "the unique identifier is a string with no meaning" is true
            of every object in the Moodle database, and with good reason. Identifiers generally should only identify and
            attaching other meaning leads to problems down the road. An example, my institution uses course IDs of the form DivisionAbbreviation-Integer, such as "ES-301". Every few years, the divisions get renamed, which changes the
            course IDs, so we can't answer simple questions like "how many students have taken this course?". Some took ES-301,
            others took the same curriculum as KP-301, and before that EN-301. So yeah, "the identifier has no meaning" is a
            good thing, I believe.

            Show
            raymor Ray Morris added a comment - - edited What do you have in mind in terms of #6, "It is a pull model (question bank asks all plugins if they have a column type) but push might be better (plugins inform the question bank if they provide column types)"? I'm not sure how that might be implemented in this context, and it affects everything else mentioned. 1. It assumes that each local plugin will add 0 or 1 other column to the question bank. Good point, and a simple fix, though the form that takes depends on #6. 2. It does not let any other plugin type change the questionbank display. True, could be better. Also true of the entire Moodle plugin system. All of the Moodle plugin functions take a plugin type argument, unfortunately. Reports plugins, enroll plugins, auth plugins etc. probably won't be adding columns to question bank, so maybe they don't need the ability to? Sny suggestion on something you think would be better, perhaps covered under #6? 3. It assumes that if the code of the local plugin wants to add a column to the question bank, then that is the right thing to do for all users. I see it this as "recognizes that if a plugin decides to add a column, the plugin is responsible for deciding when it should do so". The plugin knows what it's settings are. 4. It only changes the stand-alone view of the question bank. There is no way to change the view of the question bank in the quiz. I'm not sure I understand. Does this refer to the function that quiz_question_bank_view re-implements without calling parent::, wanted_columns()? If quiz_question_bank_view wants the full functionality of question_bank_view, should it not call $parent::wanted_columns() when it overrides that function? If not, can the same four-line patch not be copied over? 5. It assumes the local plugin will define the column type in lib.php. It does not use the new class auto-loading mechanism. Okay. I've read the new http://docs.moodle.org/dev/Automatic_class_loading , implementation again depends on #6. 6. It is a pull model (question bank asks all plugins if they have a column type) but push might be better (plugins inform the question bank if they provide column types). What do you have in mind for this? > It means that the unique identifier used to refer to the column type is a string with no meaning. ... > to use the class names ... is horribly verbose, and makes it hard to change the class names in future, > which I just did for the quiz columns. Hmm. I strongly suspect you got it right the first time. "the unique identifier is a string with no meaning" is true of every object in the Moodle database, and with good reason. Identifiers generally should only identify and attaching other meaning leads to problems down the road. An example, my institution uses course IDs of the form DivisionAbbreviation-Integer, such as "ES-301". Every few years, the divisions get renamed, which changes the course IDs, so we can't answer simple questions like "how many students have taken this course?". Some took ES-301, others took the same curriculum as KP-301, and before that EN-301. So yeah, "the identifier has no meaning" is a good thing, I believe.
            Hide
            timhunt Tim Hunt added a comment - - edited

            "the unique identifier is a string with no meaning" is true of every object in the Moodle database

            Acutally, this is not true. For example the mdl_question.qtype column stores tha acutal question type name. That translates to a folder on disc questoin/types/..., and a class name qtype_...

            In the config_plugins (and other) tables, the plugin/component columns stores a frankenstyle component name like local_myhack, mod_quiz or qtype_multichoice.

            Where possible this is good and self-documenting.

            OK, here is a possible model that is intermediate between your original code, and my horribly verbose suggestion:

            A. For defining the columns to use, we add a new setting $CFG->questionbankcolumns. We either provide no UI for that (just document it in config-dist.php) or we provide a crude text-input UI on a new settings page "Question bank settings" that probably goes just after .../admin/settings.php?section=qdefaultsetting in the tree.

            B. The format of that setting is "checkbox,qtype,questionname,editaction,previewaction,moveaction,deleteaction,creatorname,modifiername" - that is the default value, but you can modify that to include an entry like local_myplugin|mycol.

            C. the mechanism around init_column_types is changes so that we do not load all columns initially, we load them only when needed (in the unit of one component at a time).

            D. wanted_columns is changed to work by parsing $CFG->questionbankcolumns. As it tries to find the required columns, when it encounters a new component-name, it calls the init_column_types replacement to load all the column types associated with that component. This method needs to be robust if $CFG->questionbankcolumns names a column type that does not exist.

            E. [component-dir]/lib.php will have a callback method [frankenstyle]_question_bank_column_types() in lib.php that will return an array of class names. We will use auto-loading to load the actual class definitions.

            F. question_bank_view::known_field_types will be a special case. We will always call that to load the standard types. Perhaps rename it to standard_column_types, and perhaps change it return an array of class names, rather than instances.

            Does that seem like a good solution?

            We won't worry about changing quiz_question_bank_view to be more configurable yet. Space is at a premium there anyway, and we can add that later if necessary.

            Show
            timhunt Tim Hunt added a comment - - edited "the unique identifier is a string with no meaning" is true of every object in the Moodle database Acutally, this is not true. For example the mdl_question.qtype column stores tha acutal question type name. That translates to a folder on disc questoin/types/..., and a class name qtype_... In the config_plugins (and other) tables, the plugin/component columns stores a frankenstyle component name like local_myhack, mod_quiz or qtype_multichoice. Where possible this is good and self-documenting. OK, here is a possible model that is intermediate between your original code, and my horribly verbose suggestion: A. For defining the columns to use, we add a new setting $CFG->questionbankcolumns. We either provide no UI for that (just document it in config-dist.php) or we provide a crude text-input UI on a new settings page "Question bank settings" that probably goes just after .../admin/settings.php?section=qdefaultsetting in the tree. B. The format of that setting is "checkbox,qtype,questionname,editaction,previewaction,moveaction,deleteaction,creatorname,modifiername" - that is the default value, but you can modify that to include an entry like local_myplugin|mycol. C. the mechanism around init_column_types is changes so that we do not load all columns initially, we load them only when needed (in the unit of one component at a time). D. wanted_columns is changed to work by parsing $CFG->questionbankcolumns. As it tries to find the required columns, when it encounters a new component-name, it calls the init_column_types replacement to load all the column types associated with that component. This method needs to be robust if $CFG->questionbankcolumns names a column type that does not exist. E. [component-dir] /lib.php will have a callback method [frankenstyle] _question_bank_column_types() in lib.php that will return an array of class names. We will use auto-loading to load the actual class definitions. F. question_bank_view::known_field_types will be a special case. We will always call that to load the standard types. Perhaps rename it to standard_column_types, and perhaps change it return an array of class names, rather than instances. Does that seem like a good solution? We won't worry about changing quiz_question_bank_view to be more configurable yet. Space is at a premium there anyway, and we can add that later if necessary.
            Hide
            raymor Ray Morris added a comment -

            Okay, sounds good. It all seems pretty clear and straightforward other than C. That's the one part
            I'll need to stare at some more and work out as I implement the other points.

            > B. The format of that setting is
            > checkbox,qtype,questionname,editaction,previewaction,moveaction,deleteaction,creatorname,modifiername

            What an aweful format. That looks like the result of GROUP_CONCAT().

            Show
            raymor Ray Morris added a comment - Okay, sounds good. It all seems pretty clear and straightforward other than C. That's the one part I'll need to stare at some more and work out as I implement the other points. > B. The format of that setting is > checkbox,qtype,questionname,editaction,previewaction,moveaction,deleteaction,creatorname,modifiername What an aweful format. That looks like the result of GROUP_CONCAT().
            Hide
            timhunt Tim Hunt added a comment -

            Well, if you want to define a whole new relational table for this (like quiz_reports) and provide a way to edit the data in it, then be my guest

            For C. Perhaps ->knowncolumntypes should become a 2D array ->knowncolumntypes[$component][$columnname] => instance of column class. It would mainly be accessed through a getter get_column_type($fullname). If $fullname does not contain '|' then look at ->knowncolumntypes['core'][$fullname]. If it does contain '|', split it into $component and $columnname. If ->knowncolumntypes[$component] is not yet initialised, call ->load_column_types_for_component($component), then return ->knowncolumntypes[$component][$columnname]. Null if that is not set.

            Show
            timhunt Tim Hunt added a comment - Well, if you want to define a whole new relational table for this (like quiz_reports) and provide a way to edit the data in it, then be my guest For C. Perhaps ->knowncolumntypes should become a 2D array ->knowncolumntypes [$component] [$columnname] => instance of column class. It would mainly be accessed through a getter get_column_type($fullname). If $fullname does not contain '|' then look at ->knowncolumntypes ['core'] [$fullname] . If it does contain '|', split it into $component and $columnname. If ->knowncolumntypes [$component] is not yet initialised, call ->load_column_types_for_component($component), then return ->knowncolumntypes [$component] [$columnname] . Null if that is not set.
            Hide
            raymor Ray Morris added a comment -

            Just to help me get my around around the structure of this code, is the intended purpose of having wantedcolumns separate from knowncolumntypes that one day the GUI could allow the user to add columns?

            If not, it's not clicking for me why we're knowing columns that we don't want.

            Show
            raymor Ray Morris added a comment - Just to help me get my around around the structure of this code, is the intended purpose of having wantedcolumns separate from knowncolumntypes that one day the GUI could allow the user to add columns? If not, it's not clicking for me why we're knowing columns that we don't want.
            Hide
            timhunt Tim Hunt added a comment -

            "one day the GUI could allow the user to add columns?" - Yes, that is the intention.

            Show
            timhunt Tim Hunt added a comment - "one day the GUI could allow the user to add columns?" - Yes, that is the intention.
            Hide
            raymor Ray Morris added a comment - - edited

            Edit — I hereby retract this comment — End edit

            I have this just about implemented as described with the the config.php setting and I'm realizing one thing.
            There's a whole lot of hokey code around making sure that plugins can't handle their own configuration.

            Maybe it makes sense to take one more look at the simplicity of the original code I posted vs "->knowncolumntypes should become a 2D array ->knowncolumntypes[$component][$columnname] => instance of column class". Most of the complexity is because we're hacking up config.php rather than letting plugins handle their own configuration of when they should add columns and when they shouldn't.

            Show
            raymor Ray Morris added a comment - - edited Edit — I hereby retract this comment — End edit I have this just about implemented as described with the the config.php setting and I'm realizing one thing. There's a whole lot of hokey code around making sure that plugins can't handle their own configuration. Maybe it makes sense to take one more look at the simplicity of the original code I posted vs "->knowncolumntypes should become a 2D array ->knowncolumntypes [$component] [$columnname] => instance of column class". Most of the complexity is because we're hacking up config.php rather than letting plugins handle their own configuration of when they should add columns and when they shouldn't.
            Hide
            raymor Ray Morris added a comment -

            Attached is a proof of concept patch implementing Tim's suggestions. It has not yet been reviewed for whitespace style, etc. Tim, would you care to have a glance at this fairly small patch which seems to work and address the issues you mentioned, without being overly complicated hopefully?

            Show
            raymor Ray Morris added a comment - Attached is a proof of concept patch implementing Tim's suggestions. It has not yet been reviewed for whitespace style, etc. Tim, would you care to have a glance at this fairly small patch which seems to work and address the issues you mentioned, without being overly complicated hopefully?
            Hide
            timhunt Tim Hunt added a comment -

            So, I am looking at https://tracker.moodle.org/secure/attachment/34183/columns3.patch

            Hmm ... do we need all this now? Given that http://docs.moodle.org/dev/Automatic_class_loading has landed, can't you just have $CFG->questionbankcolumns refer to class names, and rely on auto-loading to load them. With special case code to handle the core column types by name.

            If not, your patch looks basically OK, but there is lots of clean-up to do. Minor points that I happened to notice:

            1. I would rather keep the default in the code as an array, rather than a string.

            2. Don't use preg_split when you can explode.

            3.

            list($plugintype, $pluginname_column) = core_component::normalize_component($column_franken);
            list($pluginname, $columnname) = explode('|', $pluginname_column);

            These two lines look backwards to me.

            4. Don't use print_error('columntypesundefined', 'question', null, $errstrings); if the middle of an API function, throw an exception.

            5. return $this->knowncolumntypes[$component][$columnname]; will cause a notice if you ask for an unknown column type.

            6. get_column_type needs a PHPdoc to explain what hte contract is. Does it throw, or return null, if the type is unknown.

            7. I am suspicious that foreach ($wanted as $colname) { has changed to foreach ($wanted as $column) {.

            Show
            timhunt Tim Hunt added a comment - So, I am looking at https://tracker.moodle.org/secure/attachment/34183/columns3.patch Hmm ... do we need all this now? Given that http://docs.moodle.org/dev/Automatic_class_loading has landed, can't you just have $CFG->questionbankcolumns refer to class names, and rely on auto-loading to load them. With special case code to handle the core column types by name. If not, your patch looks basically OK, but there is lots of clean-up to do. Minor points that I happened to notice: 1. I would rather keep the default in the code as an array, rather than a string. 2. Don't use preg_split when you can explode. 3. list($plugintype, $pluginname_column) = core_component::normalize_component($column_franken); list($pluginname, $columnname) = explode('|', $pluginname_column); These two lines look backwards to me. 4. Don't use print_error('columntypesundefined', 'question', null, $errstrings); if the middle of an API function, throw an exception. 5. return $this->knowncolumntypes [$component] [$columnname] ; will cause a notice if you ask for an unknown column type. 6. get_column_type needs a PHPdoc to explain what hte contract is. Does it throw, or return null, if the type is unknown. 7. I am suspicious that foreach ($wanted as $colname) { has changed to foreach ($wanted as $column) {.
            Hide
            raymor Ray Morris added a comment - - edited

            Thanks for your comments.

            > can't you just have $CFG->questionbankcolumns refer to class names, and rely on auto-loading to load them. With special case code to handle the core column types by name.

            Sure we could, and as I have it, they may have been autoloaded. It's just less efficient and flexible to force that given that these are already "wanted".
            My understanding was that autoloading was for classes that may not be constructed. We know we need to instantiate them. We can either a) allow the code
            that already knows where they are to load them, or b) force a hunt for for them using autoloading. There's nothing in this interface saying plugins CAN'T
            use autoloading. Plugins just have the option of better performance since they know a) we'll need an instance and b) which file to include().

            Show
            raymor Ray Morris added a comment - - edited Thanks for your comments. > can't you just have $CFG->questionbankcolumns refer to class names, and rely on auto-loading to load them. With special case code to handle the core column types by name. Sure we could, and as I have it, they may have been autoloaded. It's just less efficient and flexible to force that given that these are already "wanted". My understanding was that autoloading was for classes that may not be constructed. We know we need to instantiate them. We can either a) allow the code that already knows where they are to load them, or b) force a hunt for for them using autoloading. There's nothing in this interface saying plugins CAN'T use autoloading. Plugins just have the option of better performance since they know a) we'll need an instance and b) which file to include().
            Hide
            timhunt Tim Hunt added a comment -

            "autoloading was for classes that may not be constructed" I have no idea what you mean by this?

            The evidence so far from 2.6 is that the auto-loading Petr has implemented performs better than manual file_exists and require_once.

            Show
            timhunt Tim Hunt added a comment - "autoloading was for classes that may not be constructed" I have no idea what you mean by this? The evidence so far from 2.6 is that the auto-loading Petr has implemented performs better than manual file_exists and require_once.
            Hide
            raymor Ray Morris added a comment - - edited

            That's fine if you want to use autoloading. To explain what I mean, old style, class conditional contruction:

            require tim.plugin.php;
            require helen.plugin.php;
            if ($plugin === 'coder') {
                $hunt = new tim();
            } else if ($plugin == 'actress') {
                $hunt = new helen();
            }
            

            It's inefficient to require tim.plugin.php and helen.plugin.php when you most certainly won't use both, and may use neither.
            Autoloading means you don't load a class you're not going to use, and my understanding is that's the primary point of it.
            In Moodle, most plugins aren't being used for most pages, so parsing all of those classes that aren't used on a particular page is BAD (tm).
            Autoloading is great - no need to ever see class question_bank until you're ready to display a question bank.

            However, there's a cost. Petr's code looks in five different places to see if that's where the class is.
            If you already know where the class is, such as right here in this very file, going looking for it is slow.
            (Caching helps somewhat, of course, and has it's own costs.) In terms of performance, if the columns a plugin
            returns on dependent on some condition, it should use autoloading. Autoloading would be silly for question_bank_edit_action_column
            once we know we need it, and we know it's defined right here - no need to go looking around for it. Very much like putting
            something away in the cupboard knowing that you'll need it a few second.

            Anyway, there's nothing saying plugins can't use autoloading. I just don't see a reason to force that. Why force it to be
            an autoloaded class in whoknows/classes/what/ever_maybe/something/ instead of allowing any object to be passed to the API?

            Show
            raymor Ray Morris added a comment - - edited That's fine if you want to use autoloading. To explain what I mean, old style, class conditional contruction: require tim.plugin.php; require helen.plugin.php; if ($plugin === 'coder') { $hunt = new tim(); } else if ($plugin == 'actress') { $hunt = new helen(); } It's inefficient to require tim.plugin.php and helen.plugin.php when you most certainly won't use both, and may use neither. Autoloading means you don't load a class you're not going to use, and my understanding is that's the primary point of it. In Moodle, most plugins aren't being used for most pages, so parsing all of those classes that aren't used on a particular page is BAD (tm). Autoloading is great - no need to ever see class question_bank until you're ready to display a question bank. However, there's a cost. Petr's code looks in five different places to see if that's where the class is. If you already know where the class is, such as right here in this very file, going looking for it is slow. (Caching helps somewhat, of course, and has it's own costs.) In terms of performance, if the columns a plugin returns on dependent on some condition, it should use autoloading. Autoloading would be silly for question_bank_edit_action_column once we know we need it, and we know it's defined right here - no need to go looking around for it. Very much like putting something away in the cupboard knowing that you'll need it a few second. Anyway, there's nothing saying plugins can't use autoloading. I just don't see a reason to force that. Why force it to be an autoloaded class in whoknows/classes/what/ever_maybe/something/ instead of allowing any object to be passed to the API?
            Hide
            raymor Ray Morris added a comment -

            For a concrete example of how forcing autoloading hurts flexibility, it would mean one couldn't use question_bank_question_name_text_column, for no good reason. As it is now, a plugin could return an instance of question_bank_question_name_text_column for the main question bank page. If you force autoloading, that's no longer possible.

            Show
            raymor Ray Morris added a comment - For a concrete example of how forcing autoloading hurts flexibility, it would mean one couldn't use question_bank_question_name_text_column, for no good reason. As it is now, a plugin could return an instance of question_bank_question_name_text_column for the main question bank page. If you force autoloading, that's no longer possible.
            Hide
            raymor Ray Morris added a comment -

            Now available for review is a version which addresses the issues Tim mentioned in his comment of 21/Aug/2013 and has some other cleanup. After trying it both ways, with class names and with objects, this version covers Tim's points starting with "If not, your patch looks basically OK, but ...".

            Show
            raymor Ray Morris added a comment - Now available for review is a version which addresses the issues Tim mentioned in his comment of 21/Aug/2013 and has some other cleanup. After trying it both ways, with class names and with objects, this version covers Tim's points starting with "If not, your patch looks basically OK, but ...".
            Hide
            raymor Ray Morris added a comment -

            This sample plugin adds a "last modified" date to the question bank view in order to exercise MDL-40457.

            Show
            raymor Ray Morris added a comment - This sample plugin adds a "last modified" date to the question bank view in order to exercise MDL-40457 .
            Hide
            timhunt Tim Hunt added a comment -

            If you want this peer reviewed, please click the Request peer review button, so it is clear what state the issue is in.

            Show
            timhunt Tim Hunt added a comment - If you want this peer reviewed, please click the Request peer review button, so it is clear what state the issue is in.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: https://github.com/MorrisR2/moodle/tree/MDL-40457_plugins_can_add_columns_to_question_bank Error: all the branch fields are incorrect. Nothing was checked.
            Hide
            timhunt Tim Hunt added a comment -

            Appeasing the great got CiBoT.

            Show
            timhunt Tim Hunt added a comment - Appeasing the great got CiBoT.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            • Remote branch MDL-40457_plugins_can_add_columns_to_question_bank to be integrated into upstream master
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 _plugins_can_add_columns_to_question_bank to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1938 Warning: The MDL-40457 _plugins_can_add_columns_to_question_bank branch at git://github.com/MorrisR2/moodle.git has not been rebased recently (>20 days ago). Error: The MDL-40457 _plugins_can_add_columns_to_question_bank branch at git://github.com/MorrisR2/moodle.git does not apply clean to master
            Hide
            raymor Ray Morris added a comment -

            I see cibot says I need to rebase. I thought I had. Rebasing now.

            Show
            raymor Ray Morris added a comment - I see cibot says I need to rebase. I thought I had. Rebasing now.
            Hide
            raymor Ray Morris added a comment -

            It has been rebased.

            Show
            raymor Ray Morris added a comment - It has been rebased.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 _plugins_can_add_columns_to_question_bank to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1966 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1966/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Persuaded CiBoT to run. Can you fix the issues it found?

            Show
            timhunt Tim Hunt added a comment - Persuaded CiBoT to run. Can you fix the issues it found?
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 _plugins_can_add_columns_to_question_bank to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1969 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1969/artifact/work/smurf.html
            Hide
            raymor Ray Morris added a comment -

            I have corrected the whitespace cibot indicated. I'm not sure what it's referring to on line 986, "Phpdocs for function question_bank_view::get_column_type has incomplete parameters list".

            Show
            raymor Ray Morris added a comment - I have corrected the whitespace cibot indicated. I'm not sure what it's referring to on line 986, "Phpdocs for function question_bank_view::get_column_type has incomplete parameters list".
            Hide
            timhunt Tim Hunt added a comment -

            Argument is called $columnfranken. PHPdoc refers to $column_franken. They are different.

            In any case, that is a hideous variable name. Just call it $columnname.

            Also, the return value is of type question_bank_column_base, not mixed.

            Show
            timhunt Tim Hunt added a comment - Argument is called $columnfranken. PHPdoc refers to $column_franken. They are different. In any case, that is a hideous variable name. Just call it $columnname. Also, the return value is of type question_bank_column_base, not mixed.
            Hide
            raymor Ray Morris added a comment -

            Thank you, Tim. I was looking for something tricky and missed the obvious.

            Show
            raymor Ray Morris added a comment - Thank you, Tim. I was looking for something tricky and missed the obvious.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 _plugins_can_add_columns_to_question_bank to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2074 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2074/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            1. Have you tested this? $CFG->questionbankcolumns is a string. foreach ($CFG->questionbankcolumns as $fullname) is not going to work. (Unless it is not set, in which case the code above will set it to an array.) https://github.com/MorrisR2/moodle/compare/MDL-40457_plugins_can_add_columns_to_question_bank#diff-370ca5485da04dc9496c5731d67bf38fR963

            2. Please can you also fix the $pluginfranken variable name inside the method. I would call it $component, I think.

            Show
            timhunt Tim Hunt added a comment - 1. Have you tested this? $CFG->questionbankcolumns is a string. foreach ($CFG->questionbankcolumns as $fullname) is not going to work. (Unless it is not set, in which case the code above will set it to an array.) https://github.com/MorrisR2/moodle/compare/MDL-40457_plugins_can_add_columns_to_question_bank#diff-370ca5485da04dc9496c5731d67bf38fR963 2. Please can you also fix the $pluginfranken variable name inside the method. I would call it $component, I think.
            Hide
            raymor Ray Morris added a comment - - edited

            > Have you tested this?

            It's been in production for several months.

            2. $pluginfranken has now been replaced with re-using $component, the variable which was already used for the normalized version 2 lines down.

            1. The line just above sets it to an array because it is an array based point #1 of your comment
            https://tracker.moodle.org/browse/MDL-40457?focusedCommentId=239922&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-239922

            If you prefer, I can change it to be a string, in which case it would of course need to be explode()d before use.

            Show
            raymor Ray Morris added a comment - - edited > Have you tested this? It's been in production for several months. 2. $pluginfranken has now been replaced with re-using $component, the variable which was already used for the normalized version 2 lines down. 1. The line just above sets it to an array because it is an array based point #1 of your comment https://tracker.moodle.org/browse/MDL-40457?focusedCommentId=239922&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-239922 If you prefer, I can change it to be a string, in which case it would of course need to be explode()d before use.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 _plugins_can_add_columns_to_question_bank to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2076 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2076/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Right, in the code the data should be an array. But $CFG comes from the database. It must be a string. You need to set an array in the code by doing explode() of the $CFG value.

            Show
            timhunt Tim Hunt added a comment - Right, in the code the data should be an array. But $CFG comes from the database. It must be a string. You need to set an array in the code by doing explode() of the $CFG value.
            Hide
            raymor Ray Morris added a comment -

            thou hast believed, so be it done unto thee. And his servant code was healed in the selfsame hour.

            Show
            raymor Ray Morris added a comment - thou hast believed, so be it done unto thee. And his servant code was healed in the selfsame hour.
            Hide
            timhunt Tim Hunt added a comment -

            [Y] Syntax
            [N] Whitespace
            [Y] Output
            [Y] Language
            [-] Databases
            [Y] Testing (instructions and automated tests)
            [Y] Security
            [N] Documentation
            [N] Git
            [-] Third party code
            [-] Sanity check

            1. Why does the compare URL show three commits? MDL-40313 was integrated Please fix.

            2. https://github.com/MorrisR2/moodle/commit/b4d2c6683f9b0587ca2ef9dd7017b3ee1a6d8483 the first line of the commit comment is too long.

            3. Description in the commit comment is no longer correct. Please fix. Also, is the commit comment really the best place to document this?

            I really am trying to get this submitted for integration. It may not seem like it. It woudl go quicker if you caught more of the problems yourself, rather than waiting for me to find them. http://docs.moodle.org/dev/Peer_reviewing_checklist is in the public domain.

            Show
            timhunt Tim Hunt added a comment - [Y] Syntax [N] Whitespace [Y] Output [Y] Language [-] Databases [Y] Testing (instructions and automated tests) [Y] Security [N] Documentation [N] Git [-] Third party code [-] Sanity check 1. Why does the compare URL show three commits? MDL-40313 was integrated Please fix. 2. https://github.com/MorrisR2/moodle/commit/b4d2c6683f9b0587ca2ef9dd7017b3ee1a6d8483 the first line of the commit comment is too long. 3. Description in the commit comment is no longer correct. Please fix. Also, is the commit comment really the best place to document this? I really am trying to get this submitted for integration. It may not seem like it. It woudl go quicker if you caught more of the problems yourself, rather than waiting for me to find them. http://docs.moodle.org/dev/Peer_reviewing_checklist is in the public domain.
            Hide
            raymor Ray Morris added a comment -

            It has been updated as per your comments. Thanks for that checklist link, that will come in handy and I'll be sure to use it.
            I'm not sure how those two extraneous commits ended up in that branch. Apparently I don't completely grok git just yet.

            I sure appreciate your time on this and I know it's frustrating. Please understand I'm making every effort. For example, I see what you mean where github wrapped the 75 character commit message line. However, not noticing what github does, I was going by the "commit message style" thread, in which someone by the name of Tim Hunt said:
            "Whole first line of the commit message should ideally be less that ~80chars".
            https://moodle.org/mod/forum/discuss.php?d=180415#p785321

            So while I understand and appreciate that you want everything to be perfect, please also understand in some cases I'm following your own posted guidelines and I have no way of knowing that you've since changed your mind, until you tell me.

            You mentioned the detailed commit message. Petr Škoda likes long commit messages. You like one-liners. I'll make an effort to use long ones when Petr is reviewing them and short ones when you are. The "official" docs say "a parapgraph or two as needed". Whatever - I'll do whatever you prefer, I just have to learn what that is.

            Show
            raymor Ray Morris added a comment - It has been updated as per your comments. Thanks for that checklist link, that will come in handy and I'll be sure to use it. I'm not sure how those two extraneous commits ended up in that branch. Apparently I don't completely grok git just yet. I sure appreciate your time on this and I know it's frustrating. Please understand I'm making every effort. For example, I see what you mean where github wrapped the 75 character commit message line. However, not noticing what github does, I was going by the "commit message style" thread, in which someone by the name of Tim Hunt said: "Whole first line of the commit message should ideally be less that ~80chars". https://moodle.org/mod/forum/discuss.php?d=180415#p785321 So while I understand and appreciate that you want everything to be perfect, please also understand in some cases I'm following your own posted guidelines and I have no way of knowing that you've since changed your mind, until you tell me. You mentioned the detailed commit message. Petr Škoda likes long commit messages. You like one-liners. I'll make an effort to use long ones when Petr is reviewing them and short ones when you are. The "official" docs say "a parapgraph or two as needed". Whatever - I'll do whatever you prefer, I just have to learn what that is.
            Hide
            timhunt Tim Hunt added a comment -

            Official docs on commit messages is http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages. That should be given more credence that something I wrote once 2.5 years ago. I like long commit messages - several paragraphs if necessary, but the first line needs to be short.

            What you had written was good information. So good that it would have been a pity if it was only in the commit comment where few people would see it. Anyway, we are finally there. Sumbitting for integraiton.

            Show
            timhunt Tim Hunt added a comment - Official docs on commit messages is http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages . That should be given more credence that something I wrote once 2.5 years ago. I like long commit messages - several paragraphs if necessary, but the first line needs to be short. What you had written was good information. So good that it would have been a pity if it was only in the commit comment where few people would see it. Anyway, we are finally there. Sumbitting for integraiton.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            marina Marina Glancy added a comment -

            This issue could really benefit from MDL-44078

            Show
            marina Marina Glancy added a comment - This issue could really benefit from MDL-44078
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            PS: I really think we have to normalize callbacks (aka, proper hooks) at some point. Surely worth considering it after 2.7 LTS release (hoping we can break some things then, not sure).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! PS: I really think we have to normalize callbacks (aka, proper hooks) at some point. Surely worth considering it after 2.7 LTS release (hoping we can break some things then, not sure).
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Sorry Ray,

            I don't see tags under "Course administration -> Question bank"

            I installed local plugin as suggested and created question and added official tag. But there is no difference in navigation tree under "Question bank".

            Not sure if I am missing something, failing this for now.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Sorry Ray, I don't see tags under "Course administration -> Question bank" I installed local plugin as suggested and created question and added official tag. But there is no difference in navigation tree under "Question bank". Not sure if I am missing something, failing this for now.
            Hide
            marina Marina Glancy added a comment -

            Damyon posted on another issue:
            https://tracker.moodle.org/browse/MDL-44141?focusedCommentId=278184&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-278184

            Fatal error: Call to a member function get_name() on a non-object in /home/damyonw/Documents/Moodle/integration/master/moodle/question/editlib.php on line 1047
            Call Stack
            #	Time	Memory	Function	Location
            1	0.0019	237752	{main}( )	../edit.php:0
            2	0.0805	5954000	quiz_question_bank_view->__construct( )	../edit.php:152
            3	0.0805	5954504	question_bank_view->__construct( )	../editlib.php:1110
            4	0.0823	6552816	question_bank_view->init_columns( )	../editlib.php:934

            Show
            marina Marina Glancy added a comment - Damyon posted on another issue: https://tracker.moodle.org/browse/MDL-44141?focusedCommentId=278184&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-278184 Fatal error: Call to a member function get_name() on a non-object in /home/damyonw/Documents/Moodle/integration/master/moodle/question/editlib.php on line 1047 Call Stack # Time Memory Function Location 1 0.0019 237752 {main}( ) ../edit.php:0 2 0.0805 5954000 quiz_question_bank_view->__construct( ) ../edit.php:152 3 0.0805 5954504 question_bank_view->__construct( ) ../editlib.php:1110 4 0.0823 6552816 question_bank_view->init_columns( ) ../editlib.php:934
            Hide
            timhunt Tim Hunt added a comment -

            Raj: I agree that the testing instructions could have been worded more clearly, but it was not difficult if you bothered read the issue description.

            1. In any course, go to Question bank in the course settings.
            2. Verify there is a column displaying the question tags.

            I don't know where the error came from.

            Show
            timhunt Tim Hunt added a comment - Raj: I agree that the testing instructions could have been worded more clearly, but it was not difficult if you bothered read the issue description. 1. In any course, go to Question bank in the course settings. 2. Verify there is a column displaying the question tags. I don't know where the error came from.
            Hide
            timhunt Tim Hunt added a comment -

            Oh, I see. Ray subtly changed the init_columns API, so that $wanted changes from an array of strings to an array of objects, but then did not fix the quiz. Nor did the change get documented the change in the PHP doc comment nor in upgrade.txt.

            However, I think the API change is bad. Surely we can still pass around column names in the API, and only convert them to objects later.

            I guess this needs to be reverted for this week, unless Ray can fix it quickly. Ray is in US time-zone, I think, so give him a bit longer to respond.

            Sorry for not catching this sooner.

            Show
            timhunt Tim Hunt added a comment - Oh, I see. Ray subtly changed the init_columns API, so that $wanted changes from an array of strings to an array of objects, but then did not fix the quiz. Nor did the change get documented the change in the PHP doc comment nor in upgrade.txt. However, I think the API change is bad. Surely we can still pass around column names in the API, and only convert them to objects later. I guess this needs to be reverted for this week, unless Ray can fix it quickly. Ray is in US time-zone, I think, so give him a bit longer to respond. Sorry for not catching this sooner.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Tim,

            But I don't see tags column.

            Also, behat (mod/quiz/tests/behat/add_quiz.feature) is failing with:

            Fatal error: Call to a member function get_name() on a non-object in /var/lib/jenkins/git_repositories/master/question/editlib.php on line 1047

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Tim, But I don't see tags column. Also, behat (mod/quiz/tests/behat/add_quiz.feature) is failing with: Fatal error: Call to a member function get_name() on a non-object in /var/lib/jenkins/git_repositories/master/question/editlib.php on line 1047
            Hide
            raymor Ray Morris added a comment -

            I should have caught that error in quiz, sorry about that. That's a small and simple fix, which I've done but not pushed. While testing that and looking at it again, I saw that we probably do need to go back and address a configuration issue that had we decided to skip for now. Also, as Tim pointed out, there isn't agreement as to returning an object such as a question_bank_question_name_text_column vs returning the name of a class and requiring that the class must be autoloaded, which means question_bank_question_name_text_column, for example, couldn't be used.

            For each of the two questions, I implemented it both ways and I believe testing those implementations demonstrates which works best, but it would be good to have consensus if possible. We don't seem to be getting to consensus by posting here. Sometimes another medium of communication works better. Would it be possible to try discussing these two points by Skype or something?

            Show
            raymor Ray Morris added a comment - I should have caught that error in quiz, sorry about that. That's a small and simple fix, which I've done but not pushed. While testing that and looking at it again, I saw that we probably do need to go back and address a configuration issue that had we decided to skip for now. Also, as Tim pointed out, there isn't agreement as to returning an object such as a question_bank_question_name_text_column vs returning the name of a class and requiring that the class must be autoloaded, which means question_bank_question_name_text_column, for example, couldn't be used. For each of the two questions, I implemented it both ways and I believe testing those implementations demonstrates which works best, but it would be good to have consensus if possible. We don't seem to be getting to consensus by posting here. Sometimes another medium of communication works better. Would it be possible to try discussing these two points by Skype or something?
            Hide
            timhunt Tim Hunt added a comment -

            Yes, I am happy to discuss this on Skype or Google hang-out. tjhunt. When would suit you? I could possibly do in the next two or three hours.

            Show
            timhunt Tim Hunt added a comment - Yes, I am happy to discuss this on Skype or Google hang-out. tjhunt. When would suit you? I could possibly do in the next two or three hours.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi guys,

            I think I'm going to follow the safest path and rewrite integration.git history deleting the current commit. So you can discuss and implement it more relaxed in order to get it landing next week.

            Thanks for the effort!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi guys, I think I'm going to follow the safest path and rewrite integration.git history deleting the current commit. So you can discuss and implement it more relaxed in order to get it landing next week. Thanks for the effort!
            Hide
            timhunt Tim Hunt added a comment -

            Summary of my chat with Ray:

            1. Change the configuration setting so that it looks like a comma-separated list of franken_style-class_name, which refers to namespaces class \franken_style\question\bank\class_name_column.
            2. When the code parses the $CFG->questionbankcolumns, it does a class_exists on each one, and if it exists, uses it.
            3. We will have a special case in the code, and if the entry in the config is just class_name, that will be treated as core_question-class_name. This keeps the common case compact.
            4. We will add a separate configuration variable for the display of the question bank on the quiz editing page, since that needs to be more compact, and so configured differently.
            5. If the configuration variable is not set, the code will supply a default which is the same as we have now.
            6. If we ever do decide to build a UI for these configuration settings, then we can find all known columns by iterating over plugins looking for a classes/question/bank/ folder and its contents. Hence we can delete all the known column types code.

            In summary, this should be a nice simplification. In the mean time, Eloy is right to revert.

            Show
            timhunt Tim Hunt added a comment - Summary of my chat with Ray: Change the configuration setting so that it looks like a comma-separated list of franken_style-class_name, which refers to namespaces class \franken_style\question\bank\class_name_column. When the code parses the $CFG->questionbankcolumns, it does a class_exists on each one, and if it exists, uses it. We will have a special case in the code, and if the entry in the config is just class_name, that will be treated as core_question-class_name. This keeps the common case compact. We will add a separate configuration variable for the display of the question bank on the quiz editing page, since that needs to be more compact, and so configured differently. If the configuration variable is not set, the code will supply a default which is the same as we have now. If we ever do decide to build a UI for these configuration settings, then we can find all known columns by iterating over plugins looking for a classes/question/bank/ folder and its contents. Hence we can delete all the known column types code. In summary, this should be a nice simplification. In the mean time, Eloy is right to revert.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Thanks Tim, Ray

            reopening. As said, it never landed (have rewritten history).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Thanks Tim, Ray reopening. As said, it never landed (have rewritten history). Ciao
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            raymor Ray Morris added a comment -

            Autoloading version of the plugin to add "Last Modfied Date" column to question bank.

            Show
            raymor Ray Morris added a comment - Autoloading version of the plugin to add "Last Modfied Date" column to question bank.
            Hide
            raymor Ray Morris added a comment -

            I have pushed a new commit which revises and simplifies it.
            This is now in accordance with the discussion between Tim and myself which Tim summarized in his most recent comment.

            The commit is less drastic than the LOC would suggest, because 90%+ of the changed lines consist of moving classes from editlib.php to classes/*, where they can be autoloaded.

            By using autoloading, the known_field_types() and $knowncolumntypes are no longer required, so some complexity and 53 LOC was removed.

            Show
            raymor Ray Morris added a comment - I have pushed a new commit which revises and simplifies it. This is now in accordance with the discussion between Tim and myself which Tim summarized in his most recent comment. The commit is less drastic than the LOC would suggest, because 90%+ of the changed lines consist of moving classes from editlib.php to classes/*, where they can be autoloaded. By using autoloading, the known_field_types() and $knowncolumntypes are no longer required, so some complexity and 53 LOC was removed.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 -question_bank_plugins_columns to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2727 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2727/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Thank you very much Ray. A bit 11th hour (almost literally where I am in the UK) but this looks good.

            A assume that the missing comments that CiBoT is complaining about are either 1) overridden methods in subclasses that don't need a comment; 2) file comments that are not needed since there is only one call in the file; or 3) really missing comments in the code you just moved. It which case it is probably better not to change them in the move.

            If there are any really missing comments, please create a new issue for them. You can assign it to me.

            Submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Thank you very much Ray. A bit 11th hour (almost literally where I am in the UK) but this looks good. A assume that the missing comments that CiBoT is complaining about are either 1) overridden methods in subclasses that don't need a comment; 2) file comments that are not needed since there is only one call in the file; or 3) really missing comments in the code you just moved. It which case it is probably better not to change them in the move. If there are any really missing comments, please create a new issue for them. You can assign it to me. Submitting for integration.
            Hide
            raymor Ray Morris added a comment - - edited

            Yes, it appears that the bulk of them are in fact "3)", methods in classes that were unchanged, only moved. Also some 2) fully commented classes, with one class per file, so file-level comments would probably be redundant.

            Show
            raymor Ray Morris added a comment - - edited Yes, it appears that the bulk of them are in fact "3)", methods in classes that were unchanged, only moved. Also some 2) fully commented classes, with one class per file, so file-level comments would probably be redundant.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            raymor Ray Morris added a comment -

            For anyone interested in this, after 2.7 is released look for plugins at https://moodle.org/plugins/‎ which add columns to display the last-modified dates of the questions (so you can sort to quickly find the most recently edited ones), tags associated with questions, and the XML-Sig, for dealing with duplicate questions (ie from restore).

            Show
            raymor Ray Morris added a comment - For anyone interested in this, after 2.7 is released look for plugins at https://moodle.org/plugins/ ‎ which add columns to display the last-modified dates of the questions (so you can sort to quickly find the most recently edited ones), tags associated with questions, and the XML-Sig, for dealing with duplicate questions (ie from restore).
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Ray,

            I'm failing this presently sorry.
            The code looks good, however there is at least one regression from changes that must have landed recently.
            core_question\bank\checkbox_column::display_content contains invalid JS code.

            I attempted to just copy and paste the current JS code into that method to see if it was that simple.
            However after doing that behat [still] showed failures when running @core_question.

            01. Link matching locator "Duplicate" in the "Test question to be copied" "table_row"" not found.
            In step `When I click on "Duplicate" "link" in the "Test question to be copied" "table_row"'. # behat_general::i_click_on_in_the()
            From scenario `copy a previously created question'. # /var/www/integration/question/tests/behat/copy_questions.feature:8
            Of feature `A teacher can duplicate questions in the question bank'.

            Perhaps the fix is larger than just updating the JS, or perhaps behat tests need updating.
            Either way, this clearly needs a little more work to get these tidied up.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Ray, I'm failing this presently sorry. The code looks good, however there is at least one regression from changes that must have landed recently. core_question\bank\checkbox_column::display_content contains invalid JS code. I attempted to just copy and paste the current JS code into that method to see if it was that simple. However after doing that behat [still] showed failures when running @core_question. 01. Link matching locator "Duplicate" in the "Test question to be copied" "table_row"" not found. In step `When I click on "Duplicate" "link" in the "Test question to be copied" "table_row"'. # behat_general::i_click_on_in_the() From scenario `copy a previously created question'. # /var/www/integration/question/tests/behat/copy_questions.feature:8 Of feature `A teacher can duplicate questions in the question bank'. Perhaps the fix is larger than just updating the JS, or perhaps behat tests need updating. Either way, this clearly needs a little more work to get these tidied up. Cheers Sam
            Hide
            raymor Ray Morris added a comment - - edited

            Fixed - just a configuration setting. The test indicates that the "Duplicate" column wasn't found because it was not set as a default column in classes/bank/view.php wanted_columns(). It has now been added as a default column.

            This was a merge error with MDL-32523, which added that column type.

            PS - I apologize for the test failure - in my devel environment php -S times out after a number of tests are run, so I have trouble running all of them.

            Show
            raymor Ray Morris added a comment - - edited Fixed - just a configuration setting. The test indicates that the "Duplicate" column wasn't found because it was not set as a default column in classes/bank/view.php wanted_columns(). It has now been added as a default column. This was a merge error with MDL-32523 , which added that column type. PS - I apologize for the test failure - in my devel environment php -S times out after a number of tests are run, so I have trouble running all of them.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 -question_bank_plugins_columns to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2866 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2866/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Re-submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Re-submitting for integration.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Sorry guys, I'm still getting two behat fails here:

            01. Checkbox matching locator "my test question" in the "my test question" "table_row"" not found.
            In step `And I click on "my test question" "checkbox" in the "my test question" "table_row"'. # behat_general::i_click_on_in_the()
            From scenario `Move a question between categories via the question page'. # question/tests/behat/question_categories.feature:37
            Of feature `A teacher can put questions in categories in the question bank'. # question/tests/behat/question_categories.feature

            02. Checkbox matching locator "'A question 1 name'" not found.
            In step `Then "A question 1 name" "checkbox" should appear before "B question 2 name" "checkbox"'. # behat_general::should_appear_before()
            From scenario `Sort using question name, question type and created by sort order links'. # question/tests/behat/sort_questions.feature:8
            Of feature `A teacher can sort questions in the question bank'.

            The JavaScript code is still wrong as well, I copied and pasted across to fix this for testing so perhaps there is still something wrong there or perhaps it is somewhere else again.
            But please could you fix this on your branch.

            Before: https://github.com/MorrisR2/moodle/compare/MDL-40457-question_bank_plugins_columns#diff-370ca5485da04dc9496c5731d67bf38fL443

            After: https://github.com/MorrisR2/moodle/compare/MDL-40457-question_bank_plugins_columns#diff-a1cf60955decd3b3676131fb4d700fccR49

            Sorry again,
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Sorry guys, I'm still getting two behat fails here: 01. Checkbox matching locator "my test question" in the "my test question" "table_row"" not found. In step `And I click on "my test question" "checkbox" in the "my test question" "table_row"'. # behat_general::i_click_on_in_the() From scenario `Move a question between categories via the question page'. # question/tests/behat/question_categories.feature:37 Of feature `A teacher can put questions in categories in the question bank'. # question/tests/behat/question_categories.feature 02. Checkbox matching locator "'A question 1 name'" not found. In step `Then "A question 1 name" "checkbox" should appear before "B question 2 name" "checkbox"'. # behat_general::should_appear_before() From scenario `Sort using question name, question type and created by sort order links'. # question/tests/behat/sort_questions.feature:8 Of feature `A teacher can sort questions in the question bank'. The JavaScript code is still wrong as well, I copied and pasted across to fix this for testing so perhaps there is still something wrong there or perhaps it is somewhere else again. But please could you fix this on your branch. Before: https://github.com/MorrisR2/moodle/compare/MDL-40457-question_bank_plugins_columns#diff-370ca5485da04dc9496c5731d67bf38fL443 After: https://github.com/MorrisR2/moodle/compare/MDL-40457-question_bank_plugins_columns#diff-a1cf60955decd3b3676131fb4d700fccR49 Sorry again, Sam
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            raymor Ray Morris added a comment -

            All unit tests now pass.“ヽ(´▽`)ノ”

            The Javascript change from MDL-32729 has been integrated to fix the JavaScript error.

            Show
            raymor Ray Morris added a comment - All unit tests now pass.“ヽ(´▽`)ノ” The Javascript change from MDL-32729 has been integrated to fix the JavaScript error.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 -question_bank_plugins_columns to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2942 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2942/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Ray. Let's try the integration dance again.

            Show
            timhunt Tim Hunt added a comment - Thanks Ray. Let's try the integration dance again.
            Hide
            poltawski Dan Poltawski added a comment -

            This issue contains improvements and/or new features not allowed after freeze and so has been held. As this issue was initially sent to integration before freeze, Martin Dougiamas has the superpowers required to ask for its integration.

            Show
            poltawski Dan Poltawski added a comment - This issue contains improvements and/or new features not allowed after freeze and so has been held. As this issue was initially sent to integration before freeze, Martin Dougiamas has the superpowers required to ask for its integration.
            Hide
            timhunt Tim Hunt added a comment -

            I hope this is just a formality.

            Show
            timhunt Tim Hunt added a comment - I hope this is just a formality.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            poltawski Dan Poltawski added a comment -

            The (pretty limited) behat tests we have at the moment for questions find a problem:

            01. Moodle exception: Coding error detected, it must be fixed by a programmer: No such class exists: core_questionbankcreator_name_column More information about this error
             
                Debug info:
             
                Error code: codingerror
             
                Stack trace:
             
                line 163 of /question/classes/bank/view.php: coding_exception thrown
                line 243 of /question/classes/bank/view.php: call to core_question\bank\view->get_column_type()
                line 276 of /question/classes/bank/view.php: call to core_question\bank\view->parse_subsort()
                line 222 of /question/classes/bank/view.php: call to core_question\bank\view->init_sort_from_params()
                line 104 of /question/classes/bank/view.php: call to core_question\bank\view->init_sort()
                line 39 of /question/edit.php: call to core_question\bank\view->__construct()
                In step `And I click on "Show question text in the question list" "checkbox"'.           # behat_general::i_click_on()
                From scenario `Sort using question name, question type and created by sort order links'. # /Users/danp/moodles/im/moodle/question/tests/behat/sort_questions.feature:8
                Of feature `A teacher can sort questions in the question bank'.                          # /Users/danp/moodles/im/moodle/question/tests/behat/sort_questions.feature
             
            9 scenarios (8 passed, 1 failed)
            285 steps (281 passed, 3 skipped, 1 failed)
            3m46.653s
            

            Show
            poltawski Dan Poltawski added a comment - The (pretty limited) behat tests we have at the moment for questions find a problem: 01. Moodle exception: Coding error detected, it must be fixed by a programmer: No such class exists: core_questionbankcreator_name_column More information about this error   Debug info:   Error code: codingerror   Stack trace:   line 163 of /question/classes/bank/view.php: coding_exception thrown line 243 of /question/classes/bank/view.php: call to core_question\bank\view->get_column_type() line 276 of /question/classes/bank/view.php: call to core_question\bank\view->parse_subsort() line 222 of /question/classes/bank/view.php: call to core_question\bank\view->init_sort_from_params() line 104 of /question/classes/bank/view.php: call to core_question\bank\view->init_sort() line 39 of /question/edit.php: call to core_question\bank\view->__construct() In step `And I click on "Show question text in the question list" "checkbox"'. # behat_general::i_click_on() From scenario `Sort using question name, question type and created by sort order links'. # /Users/danp/moodles/im/moodle/question/tests/behat/sort_questions.feature:8 Of feature `A teacher can sort questions in the question bank'. # /Users/danp/moodles/im/moodle/question/tests/behat/sort_questions.feature   9 scenarios (8 passed, 1 failed) 285 steps (281 passed, 3 skipped, 1 failed) 3m46.653s
            Hide
            poltawski Dan Poltawski added a comment -

            Reopening due to the behat failure - hopefully more testing can be done before integration too

            Show
            poltawski Dan Poltawski added a comment - Reopening due to the behat failure - hopefully more testing can be done before integration too
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            raymor Ray Morris added a comment -

            All tests now pass.

            Show
            raymor Ray Morris added a comment - All tests now pass.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40457

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40457 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-40457 -question_bank_plugins_columns to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3978 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3978/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Looking at https://github.com/MorrisR2/moodle/compare/MDL-40457-question_bank_plugins_columns, this branch has not been properly rebased. Please can you fix that. Thanks.

            Show
            timhunt Tim Hunt added a comment - Looking at https://github.com/MorrisR2/moodle/compare/MDL-40457-question_bank_plugins_columns , this branch has not been properly rebased. Please can you fix that. Thanks.
            Hide
            raymor Ray Morris added a comment -

            It has been rebased.

            Show
            raymor Ray Morris added a comment - It has been rebased.
            Hide
            timhunt Tim Hunt added a comment -

            Re-submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Re-submitting for integration.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            timhunt Tim Hunt added a comment -

            This is blocking other question changes. Increasing priority.

            Show
            timhunt Tim Hunt added a comment - This is blocking other question changes. Increasing priority.
            Hide
            poltawski Dan Poltawski added a comment -

            Sorry. I said I would look at this but have since discovered that this issue has been brought up in the context of a resolution on the 'hooks' discussions: https://moodle.org/mod/forum/discuss.php?d=261673 Someone should've commuted here about this.

            In my view this can land as it is because we have existing code doing the same thing e.g. $CFG->moodlepageclass and then sorted out later. But as I am not supposed to be spending my time on integrating this week I can't spend too much time arguing this case. I hope the future integrator will make concessions for the amount of to-and froing this issue has had

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
            Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            poltawski Dan Poltawski added a comment - Sorry. I said I would look at this but have since discovered that this issue has been brought up in the context of a resolution on the 'hooks' discussions: https://moodle.org/mod/forum/discuss.php?d=261673 Someone should've commuted here about this. In my view this can land as it is because we have existing code doing the same thing e.g. $CFG->moodlepageclass and then sorted out later. But as I am not supposed to be spending my time on integrating this week I can't spend too much time arguing this case. I hope the future integrator will make concessions for the amount of to-and froing this issue has had The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            marina Marina Glancy added a comment - - edited

            Just expressing my opinion here about not-attractiveness of the code in wanted_columns(). Also lack of documentation of the $CFG variable has already been pointed out. Also question/upgrade.txt tells to implement callback but I can't find anything in code actually calling this callback.

            I have linked this issue to MDL-46155 and will recommend to implement here a search for classes among installed plugins. It could be done by questioning the existing plugins:

            core_component::find_classes_in_plugins('*', 'question', 'question_bank_column_base', true);
            

            This will locate all classes extending question_bank_column_base located in <plugindir>/classes/question/
            Since 'question' is a valid core api name, it is allowed to have subfolder classes/question/

            Show
            marina Marina Glancy added a comment - - edited Just expressing my opinion here about not-attractiveness of the code in wanted_columns(). Also lack of documentation of the $CFG variable has already been pointed out. Also question/upgrade.txt tells to implement callback but I can't find anything in code actually calling this callback. I have linked this issue to MDL-46155 and will recommend to implement here a search for classes among installed plugins. It could be done by questioning the existing plugins: core_component::find_classes_in_plugins('*', 'question', 'question_bank_column_base', true); This will locate all classes extending question_bank_column_base located in <plugindir>/classes/question/ Since 'question' is a valid core api name, it is allowed to have subfolder classes/question/
            Hide
            raymor Ray Morris added a comment -

            Thank for pointing out that upgrade.txt wasn't updated to match the design we ended up with. That has been updated.

            Certainly the new stuff proposed in MDL-46155 could help make this code cleaner, if and when those new functions get implemented. I've added a watch to your issue so that after the new API is discussed and hopefully integrated, I can use it to make this a bit more elegant.

            Show
            raymor Ray Morris added a comment - Thank for pointing out that upgrade.txt wasn't updated to match the design we ended up with. That has been updated. Certainly the new stuff proposed in MDL-46155 could help make this code cleaner, if and when those new functions get implemented. I've added a watch to your issue so that after the new API is discussed and hopefully integrated, I can use it to make this a bit more elegant.
            Hide
            marina Marina Glancy added a comment -

            Hi Ray, I also noticed that changes are listed under 2.7 in upgrade.txt
            So, where can this $CFG variable be setup? There is no UI to do it. When say that "users can set it", does it mean students can set it too? Or managers? From the code it looks like only admin can set it in config.php but it's never mentioned. Should not you specify default value as well so admin does not need to grep the code?
            Also it throws coding exception if some class name is missing. This looks too strict.

            But saying all this I still don't find your approach to have a good coding style, sorry.

            Show
            marina Marina Glancy added a comment - Hi Ray, I also noticed that changes are listed under 2.7 in upgrade.txt So, where can this $CFG variable be setup? There is no UI to do it. When say that "users can set it", does it mean students can set it too? Or managers? From the code it looks like only admin can set it in config.php but it's never mentioned. Should not you specify default value as well so admin does not need to grep the code? Also it throws coding exception if some class name is missing. This looks too strict. But saying all this I still don't find your approach to have a good coding style, sorry.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Ray, i've integrated this to master now.

            Note that its fair to say there was considerable disagreement between between integrators about this $CFG hook and the method of searching for classes in plugins and in fact a meeting is scheduled next week to discuss this type of issue and also the plugin to plugin communication ( https://moodle.org/mod/forum/discuss.php?d=261673).

            In many circumstances I probably would've pushed back until we had come to a conclusion about this. But I think this kind of $CFG approach has been in use before and I don't like the amount of delay this issue has experienced. The fact that we don't have a definitive solution for that problem yet and fact Tim is in support of this approach led to integrating it with that question still around.

            Note that if we do come up with the 'recommended' approach for doing this in the next few weeks, I think we should revisit this $CFG flag.

            Show
            poltawski Dan Poltawski added a comment - Thanks Ray, i've integrated this to master now. Note that its fair to say there was considerable disagreement between between integrators about this $CFG hook and the method of searching for classes in plugins and in fact a meeting is scheduled next week to discuss this type of issue and also the plugin to plugin communication ( https://moodle.org/mod/forum/discuss.php?d=261673 ). In many circumstances I probably would've pushed back until we had come to a conclusion about this. But I think this kind of $CFG approach has been in use before and I don't like the amount of delay this issue has experienced. The fact that we don't have a definitive solution for that problem yet and fact Tim is in support of this approach led to integrating it with that question still around. Note that if we do come up with the 'recommended' approach for doing this in the next few weeks, I think we should revisit this $CFG flag.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Dan.

            I am happy to help with any rework required. Hopefully we can get a good solution to these issues agreed before the 2.8 freeze.

            In the mean-time, this issue represents a good step forwards. Thanks to Ray, and you.

            Show
            timhunt Tim Hunt added a comment - Thanks Dan. I am happy to help with any rework required. Hopefully we can get a good solution to these issues agreed before the 2.8 freeze. In the mean-time, this issue represents a good step forwards. Thanks to Ray, and you.
            Hide
            raymor Ray Morris added a comment -

            Thanks, Dan. Certainly we can rework the configuration side when a decision is made as to how that should be done.

            In the meantime, we don't miss out on useful functionality. This is good, I think. If we only accepted perfection, Moodle would still be about 30 lines of code, awaiting the resolution of the debate whether to balance braces by putting them on a line by themselves or save screen space by putting the opening brace at the end of the line.

            Show
            raymor Ray Morris added a comment - Thanks, Dan. Certainly we can rework the configuration side when a decision is made as to how that should be done. In the meantime, we don't miss out on useful functionality. This is good, I think. If we only accepted perfection, Moodle would still be about 30 lines of code, awaiting the resolution of the debate whether to balance braces by putting them on a line by themselves or save screen space by putting the opening brace at the end of the line.
            Hide
            abgreeve Adrian Greeve added a comment -

            The first few steps of the test worked. The additional column is showing, but these changes are causing a regression.

            When I go to edit a quiz, if I change the category in the question bank I get the following error:

            A required parameter (cmid) was missing

            Debug info:
            Error code: missingparam
            Stack trace:

            • line 463 of /lib/setuplib.php: moodle_exception thrown
            • line 545 of /lib/moodlelib.php: call to print_error()
            • line 272 of /question/editlib.php: call to required_param()
            • line 120 of /mod/quiz/edit.php: call to question_edit_setup()
            Show
            abgreeve Adrian Greeve added a comment - The first few steps of the test worked. The additional column is showing, but these changes are causing a regression. When I go to edit a quiz, if I change the category in the question bank I get the following error: A required parameter (cmid) was missing Debug info: Error code: missingparam Stack trace: line 463 of /lib/setuplib.php: moodle_exception thrown line 545 of /lib/moodlelib.php: call to print_error() line 272 of /question/editlib.php: call to required_param() line 120 of /mod/quiz/edit.php: call to question_edit_setup()
            Hide
            poltawski Dan Poltawski added a comment -

            Found this problem when trying to test MDL-46093 too

            Show
            poltawski Dan Poltawski added a comment - Found this problem when trying to test MDL-46093 too
            Hide
            timhunt Tim Hunt added a comment -

            The failure is acutually caused by MDL-46163

            Show
            timhunt Tim Hunt added a comment - The failure is acutually caused by MDL-46163
            Hide
            poltawski Dan Poltawski added a comment -

            Sending this issue back to testing

            Show
            poltawski Dan Poltawski added a comment - Sending this issue back to testing
            Hide
            raymor Ray Morris added a comment -

            Marina, There is a separate issue for a clean way of doing this kind of configuration, probably a GUI of some kind. Stay tuned for more.

            Show
            raymor Ray Morris added a comment - Marina, There is a separate issue for a clean way of doing this kind of configuration, probably a GUI of some kind. Stay tuned for more.
            Hide
            abgreeve Adrian Greeve added a comment -

            When I get to step four I now get this error:

            Coding error detected, it must be fixed by a programmer: No such class exists: local_questionbankmodified_question_bank_column

            More information about this error
            Debug info:
            Error code: codingerror
            Stack trace:

            line 143 of /question/classes/bank/view.php: coding_exception thrown
            line 103 of /question/classes/bank/view.php: call to core_question\bank\view->wanted_columns()
            line 39 of /question/edit.php: call to core_question\bank\view->__construct()

            Show
            abgreeve Adrian Greeve added a comment - When I get to step four I now get this error: Coding error detected, it must be fixed by a programmer: No such class exists: local_questionbankmodified_question_bank_column More information about this error Debug info: Error code: codingerror Stack trace: line 143 of /question/classes/bank/view.php: coding_exception thrown line 103 of /question/classes/bank/view.php: call to core_question\bank\view->wanted_columns() line 39 of /question/edit.php: call to core_question\bank\view->__construct()
            Hide
            marina Marina Glancy added a comment -

            I told you that throwing exception was overkill!

            Show
            marina Marina Glancy added a comment - I told you that throwing exception was overkill!
            Hide
            marina Marina Glancy added a comment -

            changing status to back to testing because it was not a real problem

            Show
            marina Marina Glancy added a comment - changing status to back to testing because it was not a real problem
            Hide
            abgreeve Adrian Greeve added a comment -

            Okay, I finally managed to make it through a run of the testing instructions with no errors.
            Thanks for your patience.
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - Okay, I finally managed to make it through a run of the testing instructions with no errors. Thanks for your patience. Test passed.
            Hide
            poltawski Dan Poltawski added a comment -

            This change is now part of Moodle! Thanks for your contribution!

            Before software can be reusable it first has to be usable.
            --Ralph Johnson

            Show
            poltawski Dan Poltawski added a comment - This change is now part of Moodle! Thanks for your contribution! Before software can be reusable it first has to be usable. --Ralph Johnson

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Nov/14