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

Question bank - question filtering API for flexibility, plugins

    Details

    • Testing Instructions:
      Hide

      To test standard functions:
      Under Course > Question Bank,add or use a question bank with some subcategories
      Ensure at least one question is deleted (hidden)
      Toggle the controls "Also show questions from sub-categories" and "Show question text in the question list"
      Ensure that each control operates as labeled

      To test plugin API:
      Install the searchbytags plugin linked to this issue
      Add some tags to some questions
      Search for questions by tags using the new UI element on the Question Bank form
      Ensure that questions which match the searched tags are shown

      Select a quiz, choose Edit Quiz, and repeat the tests with that view of the question bank.

      Show
      To test standard functions: Under Course > Question Bank,add or use a question bank with some subcategories Ensure at least one question is deleted (hidden) Toggle the controls "Also show questions from sub-categories" and "Show question text in the question list" Ensure that each control operates as labeled To test plugin API: Install the searchbytags plugin linked to this issue Add some tags to some questions Search for questions by tags using the new UI element on the Question Bank form Ensure that questions which match the searched tags are shown Select a quiz, choose Edit Quiz, and repeat the tests with that view of the question bank.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      This API adjustment makes it possible for code to search/filter questions in the question bank by any criteria. It introduces a new "question_bank_search_condition" class.

      Example usage includes plugins or future code finding questions based on tags, a text search, or finding questions of a certain type. Ie, show all true/false questions in a category, all questions tagged "PHP", or all questions which mention "SimpleTest".

      For illustration, the category filter and the "show old questions" filter have been converted to question_bank_search_condition subclasses. A test subclass to find questions based on tags also exists using this API.

        Gliffy Diagrams

        1. questionbank_quiz_compare.png
          267 kB
        2. Screen Shot 2013-07-18 at 1.39.39 PM.png
          24 kB
        3. Screen Shot 2013-07-18 at 3.19.48 PM.png
          19 kB
        4. Screen Shot 2013-07-18 at 3.19.57 PM.png
          23 kB
        5. Screen Shot 2013-08-02 at 2.49.53 PM.png
          65 kB
        6. Screen Shot 2013-08-02 at 4.40.21 PM.png
          61 kB
        7. Screen Shot 2013-08-02 at 4.56.04 PM.png
          57 kB
        8. Screen Shot 2013-08-05 at 8.58.23 AM.png
          60 kB

          Issue Links

            Activity

            Hide
            raymor Ray Morris added a comment -

            I converted the category filter as an example of the new API. I'm not sure one way or the other whether the category filter actually should end up using the new API. On the one hand, questions are ALWAYS in a category, so in all cases users will either want to filter by category, or (rarely) it will be a noop if the top level category is used. So it doesn't need to separated out as a filter. On the other hand, if it can be done as "just another filter", why not make all filters consistent.

            Show
            raymor Ray Morris added a comment - I converted the category filter as an example of the new API. I'm not sure one way or the other whether the category filter actually should end up using the new API. On the one hand, questions are ALWAYS in a category, so in all cases users will either want to filter by category, or (rarely) it will be a noop if the top level category is used. So it doesn't need to separated out as a filter. On the other hand, if it can be done as "just another filter", why not make all filters consistent.
            Hide
            timhunt Tim Hunt added a comment -

            I agree use filters for everything so it is consistent. I am very busy this week, so I may not have time to look at your code properly until next week, but when I looked before it was looking good, so carry on.

            Show
            timhunt Tim Hunt added a comment - I agree use filters for everything so it is consistent. I am very busy this week, so I may not have time to look at your code properly until next week, but when I looked before it was looking good, so carry on.
            Hide
            raymor Ray Morris added a comment -

            I was thinking there was a field to change for this purpose, but I don't see it now.
            There is a new version of this coming, so there's no need to look too closely at what is posted now.
            Since most filters will have a GUI element, I added a method question_bank_search_condition->display_options().
            Moving the GUI for category selection to question_bank_search_condition_category involves moving a lot of code.

            Looking at the code posted above might be useful for seeing how it's designed to work without being distracted by many lines of diff that are just moving the category GUI, though.

            Show
            raymor Ray Morris added a comment - I was thinking there was a field to change for this purpose, but I don't see it now. There is a new version of this coming, so there's no need to look too closely at what is posted now. Since most filters will have a GUI element, I added a method question_bank_search_condition->display_options(). Moving the GUI for category selection to question_bank_search_condition_category involves moving a lot of code. Looking at the code posted above might be useful for seeing how it's designed to work without being distracted by many lines of diff that are just moving the category GUI, though.
            Hide
            raymor Ray Morris added a comment -

            The Pull Master Diff URL has now been updated with the latest version, which includes the GUI.
            The majority of lines of changes are simply moving the category filter GUI to a filter object.

            Because the core of it was written before autoloading was merged, the current code does not yet use
            the brand new autoloading of classes, but is otherwise ready I believe. Autoloading could be added if needed, but not before I'm gone up to four days for the holiday here. I wanted to make to available for review before leaving.

            Show
            raymor Ray Morris added a comment - The Pull Master Diff URL has now been updated with the latest version, which includes the GUI. The majority of lines of changes are simply moving the category filter GUI to a filter object. Because the core of it was written before autoloading was merged, the current code does not yet use the brand new autoloading of classes, but is otherwise ready I believe. Autoloading could be added if needed, but not before I'm gone up to four days for the holiday here. I wanted to make to available for review before leaving.
            Hide
            timhunt Tim Hunt added a comment -

            I hope you had a great holiday.

            Quite a lot of this is looking good, but there are still issues:

            0. You need to rebase your branch. The list of commits on https://github.com/MorrisR2/moodle/compare/MDL-4013-question_filtering_api_c is confusing, but I think the diff makes sense.

            1. init_search_conditions suffers from the same issue we are discussing in MDL-40457. Can we use a similar fix?

            2. Coding style around build_query needs to be cleaned up. Shouls use PHPdoc comments, and one blank line beteween methods.

            3. Why two brackets in $tests[] = '((' . $searchcondition->where() .'))';

            4. Why array_unshift here? array_unshift($this->searchconditions, new question_bank_search_condition_hide(! $showhidden)); $this->searchconditions[] = ... is more normal.

            5. advancedsearch should be called display_advanced_search_form. You should use html_writer. New JavaScript code really ought to be done as YUI module. Can't you use the standard print_collapsible_region?

            6. abstract class question_bank_search_condition needs PHPdoc comments.

            7. display_category_form_checkbox should use html_writer.

            8. You should never use inline JavaScript like echo ' onchange="getElementById(\'displayoptions\').submit(); return true;" />';

            9. /// Get all the existing categories now is no longer the right inline comment style.

            Show
            timhunt Tim Hunt added a comment - I hope you had a great holiday. Quite a lot of this is looking good, but there are still issues: 0. You need to rebase your branch. The list of commits on https://github.com/MorrisR2/moodle/compare/MDL-4013-question_filtering_api_c is confusing, but I think the diff makes sense. 1. init_search_conditions suffers from the same issue we are discussing in MDL-40457 . Can we use a similar fix? 2. Coding style around build_query needs to be cleaned up. Shouls use PHPdoc comments, and one blank line beteween methods. 3. Why two brackets in $tests[] = '((' . $searchcondition->where() .'))'; 4. Why array_unshift here? array_unshift($this->searchconditions, new question_bank_search_condition_hide(! $showhidden)); $this->searchconditions[] = ... is more normal. 5. advancedsearch should be called display_advanced_search_form. You should use html_writer. New JavaScript code really ought to be done as YUI module. Can't you use the standard print_collapsible_region? 6. abstract class question_bank_search_condition needs PHPdoc comments. 7. display_category_form_checkbox should use html_writer. 8. You should never use inline JavaScript like echo ' onchange="getElementById(\'displayoptions\').submit(); return true;" />'; 9. /// Get all the existing categories now is no longer the right inline comment style.
            Hide
            raymor Ray Morris added a comment - - edited

            Excellent, thanks Tim. I'll take care of that stuff. The entire file has uses the /// comment style. My thinking was that I'd submit a separate tracker issue fixing that throughout the file, rather than fixing it only in a few places mixed in with this commit. (/// Get all existing categories is one line in the middle of existing code which has simply been moved into a class).

            To answer a couple of the questions you asked:

            > Why two brackets in $tests[] = '((' . $searchcondition->where() .'))';

            Off hand I'm not remembering exactly what the common idiom is, but generally we don't know now and plugins do no know what conditions other plugins will add. There are common cases in which the order of operations is such that conditions added by different modules can interact in surprising ways if they aren't isolated by double parens. I wish I could remember exact details, but the issue was discussed and addressed by another high profile open source project and smart people decided a double parens was the right thing.

            > Why array_unshift here? ... this->searchconditions[] is more normal.

            To put the default filters at the top, so they don't move when plugins are added.
            With unshift, adding plugins adds to the menu like this:

            Default   With plugins
            a)        a)
            b)        b)
                      1)
                      2)
                      3)
            

            Putting the defaults at the end of the list with searchconditions[] results in the defaults moving, like this:

            Default   With plugins
            a)        1)
            b)        2)
                      3)
                      a)
                      b)
            

            > init_search_conditions suffers from the same issue we are discussing in MDL-40457. Can we use a similar fix?

            We could, sure. By "similar" fix we mean a configuration variable, right? I do see a difference between this
            and MDL-40457. In this case, adding a plugin only adds an optional search field that they can choose to
            use or not, probably one that's hidden within the "show more" section. I'm not sure it makes much sense to install
            a quiz editing plugin but not allow the people editing quizzes to use it. Can we leave that one until someone
            has a need for such?

            Show
            raymor Ray Morris added a comment - - edited Excellent, thanks Tim. I'll take care of that stuff. The entire file has uses the /// comment style. My thinking was that I'd submit a separate tracker issue fixing that throughout the file, rather than fixing it only in a few places mixed in with this commit. (/// Get all existing categories is one line in the middle of existing code which has simply been moved into a class). To answer a couple of the questions you asked: > Why two brackets in $tests[] = '((' . $searchcondition->where() .'))'; Off hand I'm not remembering exactly what the common idiom is, but generally we don't know now and plugins do no know what conditions other plugins will add. There are common cases in which the order of operations is such that conditions added by different modules can interact in surprising ways if they aren't isolated by double parens. I wish I could remember exact details, but the issue was discussed and addressed by another high profile open source project and smart people decided a double parens was the right thing. > Why array_unshift here? ... this->searchconditions[] is more normal. To put the default filters at the top, so they don't move when plugins are added. With unshift, adding plugins adds to the menu like this: Default With plugins a) a) b) b) 1) 2) 3) Putting the defaults at the end of the list with searchconditions[] results in the defaults moving, like this: Default With plugins a) 1) b) 2) 3) a) b) > init_search_conditions suffers from the same issue we are discussing in MDL-40457 . Can we use a similar fix? We could, sure. By "similar" fix we mean a configuration variable, right? I do see a difference between this and MDL-40457 . In this case, adding a plugin only adds an optional search field that they can choose to use or not, probably one that's hidden within the "show more" section. I'm not sure it makes much sense to install a quiz editing plugin but not allow the people editing quizzes to use it. Can we leave that one until someone has a need for such?
            Hide
            timhunt Tim Hunt added a comment -

            If you are being consistent with the comment style of the rest of the file, then that is fine. (Obviously, I coudl not tell that from the patch.)

            I would love to see the original source of the information behind the double-bracket issue. I don't see how it can possibly make a difference at the moment.

            similarly, all the filter conditions are combined using AND. a AND b AND c ... is identical, irrespective of the order of the clauses.

            Show
            timhunt Tim Hunt added a comment - If you are being consistent with the comment style of the rest of the file, then that is fine. (Obviously, I coudl not tell that from the patch.) I would love to see the original source of the information behind the double-bracket issue. I don't see how it can possibly make a difference at the moment. similarly, all the filter conditions are combined using AND. a AND b AND c ... is identical, irrespective of the order of the clauses.
            Hide
            raymor Ray Morris added a comment - - edited

            I wish I could remember the more common one, but this is one example of
            a case when the parens matter, where a AND b != (a) AND (b):

            Assume one plugin returns the condition "false"
            another returns "true XOR true" (which is also false).
            false AND false should return false, of course.

            Combine those with AND, but no parens:
            SELECT true XOR true AND false;
            That returns TRUE, because it's equivalent to:
            SELECT true XOR (true AND false)
            Which is:
            SELECT true XOR false

            The same issue can come up with clauses containing ALL, ANY, BETWEEN, IN, LIKE, OR, SOME, and ||.
            One set of parens handles that, unless one of the clauses has it's own sets of parens. Double parens
            works in all known cases.

            • "all known cases" meaning "known to me"
            Show
            raymor Ray Morris added a comment - - edited I wish I could remember the more common one, but this is one example of a case when the parens matter, where a AND b != (a) AND (b): Assume one plugin returns the condition "false" another returns "true XOR true" (which is also false). false AND false should return false, of course. Combine those with AND, but no parens: SELECT true XOR true AND false; That returns TRUE, because it's equivalent to: SELECT true XOR (true AND false) Which is: SELECT true XOR false The same issue can come up with clauses containing ALL, ANY, BETWEEN, IN, LIKE, OR, SOME, and ||. One set of parens handles that, unless one of the clauses has it's own sets of parens. Double parens works in all known cases. "all known cases" meaning "known to me"
            Hide
            timhunt Tim Hunt added a comment -

            I know that single parens are necssary. I just can't think of a case where you need ((...))

            Show
            timhunt Tim Hunt added a comment - I know that single parens are necssary. I just can't think of a case where you need ((...))
            Hide
            raymor Ray Morris added a comment -

            Here's a simpler example:

            Filter a: true OR true
            Filter b: false

            SELECT true OR true AND false
            That's TRUE. Instead you want:
            SELECT (true OR true) AND (false)

            Show
            raymor Ray Morris added a comment - Here's a simpler example: Filter a: true OR true Filter b: false SELECT true OR true AND false That's TRUE. Instead you want: SELECT (true OR true) AND (false)
            Hide
            raymor Ray Morris added a comment - - edited

            I believe you replied to my earlier comment while I was adding this to it:

            > init_search_conditions suffers from the same issue we are discussing in MDL-40457.
            > Can we use a similar fix?

            We could, sure. By "similar" fix we mean a configuration variable, right? I do see a difference
            between this and MDL-40457. In this case, adding a plugin only adds an optional search field that
            they can choose to use or not, probably one that's hidden within the "show more" section. I'm not
            sure it makes much sense to install a quiz editing plugin but not allow the people editing quizzes
            to use it. Can we leave that one until someone has a need for such?

            If so, I think today I can quickly handle the other items you mentioned and we can have something completed.

            Show
            raymor Ray Morris added a comment - - edited I believe you replied to my earlier comment while I was adding this to it: > init_search_conditions suffers from the same issue we are discussing in MDL-40457 . > Can we use a similar fix? We could, sure. By "similar" fix we mean a configuration variable, right? I do see a difference between this and MDL-40457 . In this case, adding a plugin only adds an optional search field that they can choose to use or not, probably one that's hidden within the "show more" section. I'm not sure it makes much sense to install a quiz editing plugin but not allow the people editing quizzes to use it. Can we leave that one until someone has a need for such? If so, I think today I can quickly handle the other items you mentioned and we can have something completed.
            Hide
            timhunt Tim Hunt added a comment -

            Yes. We overlapped with our cmments.

            I agree with you that these search conditions do not need to be configurable. All installed ones can be shown. However, that presents a problem. For the columns, the list of columns was a useful starting point, and meant that only needed to ask configured plugins to declare their columns. Without that, are we stuck with polling all plug-ins again? How much of a performance problem is that?

            I guess, for now, just poll all local plug-ins, but let each plug-in return a list of search condition classes, not just one. Does that seem sensible?

            Show
            timhunt Tim Hunt added a comment - Yes. We overlapped with our cmments. I agree with you that these search conditions do not need to be configurable. All installed ones can be shown. However, that presents a problem. For the columns, the list of columns was a useful starting point, and meant that only needed to ask configured plugins to declare their columns. Without that, are we stuck with polling all plug-ins again? How much of a performance problem is that? I guess, for now, just poll all local plug-ins, but let each plug-in return a list of search condition classes, not just one. Does that seem sensible?
            Hide
            raymor Ray Morris added a comment -

            Sounds sensible to me, thanks. It may be that get_plugin_list_with_class() requires a type precisely
            BECAUSE that solves performance concerns. There will rarely (never?) be hundreds of local plugins, so
            checking all 2-3 of them won't take long. I haven't tested that, but it sounds reasonable.

            Show
            raymor Ray Morris added a comment - Sounds sensible to me, thanks. It may be that get_plugin_list_with_class() requires a type precisely BECAUSE that solves performance concerns. There will rarely (never?) be hundreds of local plugins, so checking all 2-3 of them won't take long. I haven't tested that, but it sounds reasonable.
            Hide
            timhunt Tim Hunt added a comment -

            In the OU code-base, we have 49 local plugins. (!)

            The perfomance fix would be to cache the list of search classes in MUC some-how, but I don't think we should bother. This work will only be done when a teacher is looking at the question bank, and it will probably be quicker than the DB queries. (file_exist checks are very fast, because operating systems cache directory listings.)

            Show
            timhunt Tim Hunt added a comment - In the OU code-base, we have 49 local plugins. (!) The perfomance fix would be to cache the list of search classes in MUC some-how, but I don't think we should bother. This work will only be done when a teacher is looking at the question bank, and it will probably be quicker than the DB queries. (file_exist checks are very fast, because operating systems cache directory listings.)
            Hide
            raymor Ray Morris added a comment - - edited

            That's good to know.

            > file_exist checks are very fast, because operating systems cache directory listings.

            Yep. It used to be that Linux didn't use the cache in the false case, to see that a file didn't exist,
            but I fixed that. Who caches finfo = null? It turns out, it's sometimes nice to remember that finfo = null.

            Show
            raymor Ray Morris added a comment - - edited That's good to know. > file_exist checks are very fast, because operating systems cache directory listings. Yep. It used to be that Linux didn't use the cache in the false case, to see that a file didn't exist, but I fixed that. Who caches finfo = null? It turns out, it's sometimes nice to remember that finfo = null.
            Hide
            raymor Ray Morris added a comment - - edited

            The Pull Master Diff URL has been updated with a rebased version which addresses all of the above.

            It leaves untouched the raw HTML and inline Javascript in question_bank_view->display_category_form_checkbox()
            because I expect that function will be removed next week. It's used only by quiz_question_bank_view.

            If it's desired to deprecate it but leave it in in case it's used by other local subclasses, I'll submit a separate patch updating it to use html_writer and qbank.js.

            This new version also has some other general cleanup - removing the need to pass around several variables
            from function to function to function, etc.

            Show
            raymor Ray Morris added a comment - - edited The Pull Master Diff URL has been updated with a rebased version which addresses all of the above. It leaves untouched the raw HTML and inline Javascript in question_bank_view->display_category_form_checkbox() because I expect that function will be removed next week. It's used only by quiz_question_bank_view. If it's desired to deprecate it but leave it in in case it's used by other local subclasses, I'll submit a separate patch updating it to use html_writer and qbank.js. This new version also has some other general cleanup - removing the need to pass around several variables from function to function to function, etc.
            Hide
            timhunt Tim Hunt added a comment -

            Conceptually, this is now find, apart from the first two points below. The remaining points are trivial, but should be fixed.

            Remind me, if you don't have any additional plugins installed, does this make any visible change to the UI?

            Also, you comment "I expect that function will be removed next week". What other work is going to cause that to happen?

            1. We need testing instructions.

            2. All new JavaScript in Moodle needs to be done as YUI3 modues. http://docs.moodle.org/dev/YUI/Modules, http://docs.moodle.org/dev/YUI/Shifter.

            3. Should not hve @param here: https://github.com/MorrisR2/moodle/compare/master...MDL-4013-question_filtering_api_d#L0R1953

            4. https://github.com/MorrisR2/moodle/compare/master...MDL-4013-question_filtering_api_d#L0R1200 - should not have whitespace inside funtion arguments.

            5. Also https://github.com/MorrisR2/moodle/compare/master...MDL-4013-question_filtering_api_d#L0R1266, no space after !

            6. Class not documented: https://github.com/MorrisR2/moodle/compare/master...MDL-4013-question_filtering_api_d#L0R1990

            7. Should be two blank lines between classes in a file.

            8. Before this can be submitted for integration, it needs to be rebased down to a single commit.

            Show
            timhunt Tim Hunt added a comment - Conceptually, this is now find, apart from the first two points below. The remaining points are trivial, but should be fixed. Remind me, if you don't have any additional plugins installed, does this make any visible change to the UI? Also, you comment "I expect that function will be removed next week". What other work is going to cause that to happen? 1. We need testing instructions. 2. All new JavaScript in Moodle needs to be done as YUI3 modues. http://docs.moodle.org/dev/YUI/Modules , http://docs.moodle.org/dev/YUI/Shifter . 3. Should not hve @param here: https://github.com/MorrisR2/moodle/compare/master...MDL-4013-question_filtering_api_d#L0R1953 4. https://github.com/MorrisR2/moodle/compare/master...MDL-4013-question_filtering_api_d#L0R1200 - should not have whitespace inside funtion arguments. 5. Also https://github.com/MorrisR2/moodle/compare/master...MDL-4013-question_filtering_api_d#L0R1266 , no space after ! 6. Class not documented: https://github.com/MorrisR2/moodle/compare/master...MDL-4013-question_filtering_api_d#L0R1990 7. Should be two blank lines between classes in a file. 8. Before this can be submitted for integration, it needs to be rebased down to a single commit.
            Hide
            raymor Ray Morris added a comment -

            Thanks Tim, I'll take care of those things. If no plugins are installed, it makes no visible changes to the UI.

            It does provide the opportunity make it a little cleaner by placing "Also show questions from sub-categories" or "Also show old questions" under "Show more" if desired. The code as shown doesn't do so, but I did as a test and it would take ten seconds to do.

            > Also, you comment "I expect that function will be removed next week".
            > What other work is going to cause that to happen?

            The only known caller of that function is it's subclass, quiz_question_bank_view.
            In order for quiz_question_bank_view to inherit the the new functionality, it'll need a small patch.
            That small patch will remove the call to display_category_form_checkbox.

            That patch should be trivial to review because it is just a small chunk from this already reviewed issue.
            Where quiz_question_bank_view re-implements a method in question_bank_view, it'll get an adjustment
            similar to what we're doing in question_bank_view.

            Of course, if you think people may have local plugins that subclass question_bank_view and call that
            function, it can be left as is - it just won't be called from any standard Moodle code.

            Show
            raymor Ray Morris added a comment - Thanks Tim, I'll take care of those things. If no plugins are installed, it makes no visible changes to the UI. It does provide the opportunity make it a little cleaner by placing "Also show questions from sub-categories" or "Also show old questions" under "Show more" if desired. The code as shown doesn't do so, but I did as a test and it would take ten seconds to do. > Also, you comment "I expect that function will be removed next week". > What other work is going to cause that to happen? The only known caller of that function is it's subclass, quiz_question_bank_view. In order for quiz_question_bank_view to inherit the the new functionality, it'll need a small patch. That small patch will remove the call to display_category_form_checkbox. That patch should be trivial to review because it is just a small chunk from this already reviewed issue. Where quiz_question_bank_view re-implements a method in question_bank_view, it'll get an adjustment similar to what we're doing in question_bank_view. Of course, if you think people may have local plugins that subclass question_bank_view and call that function, it can be left as is - it just won't be called from any standard Moodle code.
            Hide
            timhunt Tim Hunt added a comment -

            I think you should do the fixes to quiz_question_bank_view as part of this issue.

            You are right about the possibility of other plugins subclassing question_bank_view. Don't remove any methods. Instead @deprecate them (http://docs.moodle.org/dev/Deprecation#Step_1._Immediate_action), and descrive the API changes in question/upgrade.txt.

            (question/upgrade.txt does not exist yet. I am about to create it as part of MDL-35053 but that will not be integrated yet. Let me sort out those merge conflicts when they happen.)

            Show
            timhunt Tim Hunt added a comment - I think you should do the fixes to quiz_question_bank_view as part of this issue. You are right about the possibility of other plugins subclassing question_bank_view. Don't remove any methods. Instead @deprecate them ( http://docs.moodle.org/dev/Deprecation#Step_1._Immediate_action ), and descrive the API changes in question/upgrade.txt. (question/upgrade.txt does not exist yet. I am about to create it as part of MDL-35053 but that will not be integrated yet. Let me sort out those merge conflicts when they happen.)
            Hide
            raymor Ray Morris added a comment -

            Correction - the code you were looking at does put the recurse and show hidden options under "Show more". So that's the UI change - it's slightly cleaner because those checkboxes don't show by default.
            It's easy to move them to be visible by default or not, whichever you think. Personally I like the cleaner look, but your call.

            Show
            raymor Ray Morris added a comment - Correction - the code you were looking at does put the recurse and show hidden options under "Show more". So that's the UI change - it's slightly cleaner because those checkboxes don't show by default. It's easy to move them to be visible by default or not, whichever you think. Personally I like the cleaner look, but your call.
            Hide
            timhunt Tim Hunt added a comment - - edited

            Hmm..., well at the moment the UI is not good enough.

            • The link says "Show more ..." but since there is nothing there to start with, the word 'more' does not make any sense.
            • The 'Show question text' option has got lost. We must keep that.

            So, I am not against it in principle, but it really needs to be an improvement, not a regression.

            Show
            timhunt Tim Hunt added a comment - - edited Hmm..., well at the moment the UI is not good enough. The link says "Show more ..." but since there is nothing there to start with, the word 'more' does not make any sense. The 'Show question text' option has got lost. We must keep that. So, I am not against it in principle, but it really needs to be an improvement, not a regression.
            Hide
            raymor Ray Morris added a comment -

            Thanks, I hadn't noticed that "show question text" had vanished. I'll fix that.
            We can certainly have one or more of recurse, show hidden (deleted), or "show question text" be visible by default if you think "Show more" needs a checkbox before it.

            Of course the category selection filter is there, so "Show more" does make sense if you consider that is part of the form.

            Show
            raymor Ray Morris added a comment - Thanks, I hadn't noticed that "show question text" had vanished. I'll fix that. We can certainly have one or more of recurse, show hidden (deleted), or "show question text" be visible by default if you think "Show more" needs a checkbox before it. Of course the category selection filter is there, so "Show more" does make sense if you consider that is part of the form.
            Hide
            timhunt Tim Hunt added a comment -

            I think "Show more search tools ..." or something is probably the way to make it clearer. Except that show question text is not a search tool. Also, is a link the right look? I know you have copied how it looks in moodleforms, but another alternative is how it looks on the add/remove group memebers page - the search options there.

            So basically it is a good idea, but we need to get the usability right, and I don' think we have succeeded with that yet.

            Show
            timhunt Tim Hunt added a comment - I think "Show more search tools ..." or something is probably the way to make it clearer. Except that show question text is not a search tool. Also, is a link the right look? I know you have copied how it looks in moodleforms, but another alternative is how it looks on the add/remove group memebers page - the search options there. So basically it is a good idea, but we need to get the usability right, and I don' think we have succeeded with that yet.
            Hide
            raymor Ray Morris added a comment - - edited

            If you prefer the little arrow like the groups search options, I'm fine with that.
            Same on the wording, if you think of something you think is better, let me know what you'd like it to say.

            For my part, for UI especially I strongly believe "people don't want better standards, they want standard standards". QWERTY is a HORRIBLE keyboard layout, nearly the worst you could design, but it's the standard, so presenting the user with anything but QWERTY is error.

            Moodleforms is the standard for the Moodle UI. Moodle uses a "Show more" link now. I'm sure it could be done better, but PCs use QWERTY and Moodle uses "Show more". That's my take on it.

            To me, to have one part of Moodle not follow the standard would be somewhat like Microsoft Word using Dvorak - this PC uses QWERTY, unless you're in a Word document. (Even though Dvorak is objectively better).

            PS - I think stare decisis also applies. If we re-visit all of the decisions each time a form needs to show more, or re-consider how we handle strings each time a string is needed, etc. we'll never get anything done. At some point, you just have to call a question settled and move on.

            Show
            raymor Ray Morris added a comment - - edited If you prefer the little arrow like the groups search options, I'm fine with that. Same on the wording, if you think of something you think is better, let me know what you'd like it to say. For my part, for UI especially I strongly believe "people don't want better standards, they want standard standards". QWERTY is a HORRIBLE keyboard layout, nearly the worst you could design, but it's the standard, so presenting the user with anything but QWERTY is error. Moodleforms is the standard for the Moodle UI. Moodle uses a "Show more" link now. I'm sure it could be done better, but PCs use QWERTY and Moodle uses "Show more". That's my take on it. To me, to have one part of Moodle not follow the standard would be somewhat like Microsoft Word using Dvorak - this PC uses QWERTY, unless you're in a Word document. (Even though Dvorak is objectively better). PS - I think stare decisis also applies. If we re-visit all of the decisions each time a form needs to show more, or re-consider how we handle strings each time a string is needed, etc. we'll never get anything done. At some point, you just have to call a question settled and move on.
            Hide
            timhunt Tim Hunt added a comment -

            I actually think this is not something we can answer by having a debate between two developers. We need to know what works for users. I think short text 'Show more...' works in forms, because the context is clear. I think that in the question bank, the context is vaguer, so we need more words.

            Show
            timhunt Tim Hunt added a comment - I actually think this is not something we can answer by having a debate between two developers. We need to know what works for users. I think short text 'Show more...' works in forms, because the context is clear. I think that in the question bank, the context is vaguer, so we need more words.
            Hide
            raymor Ray Morris added a comment - - edited

            Again, if you get some other text from users that you think is better, it takes several seconds to update the text. I'm not debating - I'm more than happy to use whatever text you think is best.

            As you know, the discussion about what wording to use went on for a couple of months, since at least March. Four months later, if you want to change it, fine, but it's up to you to ask some users or whatever you want to do and propose some new text. Four months is as long as I'm able to commit to that particular discussion. If you want to change it now, you propose some new wording.

            Show
            raymor Ray Morris added a comment - - edited Again, if you get some other text from users that you think is better, it takes several seconds to update the text. I'm not debating - I'm more than happy to use whatever text you think is best. As you know, the discussion about what wording to use went on for a couple of months, since at least March. Four months later, if you want to change it, fine, but it's up to you to ask some users or whatever you want to do and propose some new text. Four months is as long as I'm able to commit to that particular discussion. If you want to change it now, you propose some new wording.
            Hide
            raymor Ray Morris added a comment -

            Before and after screenshots.
            The first one is "before", the last two are default and with "Show more" clicked.

            Show
            raymor Ray Morris added a comment - Before and after screenshots. The first one is "before", the last two are default and with "Show more" clicked.
            Hide
            raymor Ray Morris added a comment -

            "After" screenshots

            Show
            raymor Ray Morris added a comment - "After" screenshots
            Hide
            raymor Ray Morris added a comment -

            "after" screenshots

            Show
            raymor Ray Morris added a comment - "after" screenshots
            Hide
            raymor Ray Morris added a comment -

            I've addressed all of the above, other than adding mod/quiz/quiz_question_bank_view.

            Regarding adjusting quiz_question_bank_view, I see MDL-33071 should be integrated shortly, so I'll hold
            off on quiz work for the weekend to avoid a difficult merge. Is there any other significant restructuring effort, anything that would make merging difficult?

            Please have a look at the changed signature for protected function display_options.
            The first two arguments are no longer needed / used. Is that okay as is, do you think, or should
            it be stubbed the way I did with build_query_sql(...) being replaced by build_query()?

            Next up - mechanical style fixes like replacing triple /// comments with // to get rid of the hundreds of warnings from the style checker.

            Show
            raymor Ray Morris added a comment - I've addressed all of the above, other than adding mod/quiz/quiz_question_bank_view. Regarding adjusting quiz_question_bank_view, I see MDL-33071 should be integrated shortly, so I'll hold off on quiz work for the weekend to avoid a difficult merge. Is there any other significant restructuring effort, anything that would make merging difficult? Please have a look at the changed signature for protected function display_options. The first two arguments are no longer needed / used. Is that okay as is, do you think, or should it be stubbed the way I did with build_query_sql(...) being replaced by build_query()? Next up - mechanical style fixes like replacing triple /// comments with // to get rid of the hundreds of warnings from the style checker.
            Hide
            timhunt Tim Hunt added a comment -

            Please do quiz_question_bank_view. I will hold off on MDL-33071 until this is integrated.

            Please don't fix unrelated code-checker issues. Only change lines that are already affected by this patch.

            I am about to look at the display_options question.

            Show
            timhunt Tim Hunt added a comment - Please do quiz_question_bank_view. I will hold off on MDL-33071 until this is integrated. Please don't fix unrelated code-checker issues. Only change lines that are already affected by this patch. I am about to look at the display_options question.
            Hide
            raymor Ray Morris added a comment - - edited

            After addressing my Git issues, the new revision is now available, rebased.
            1332 / 1341 is the line I was seeking feedback on.

            I'd like to have this reviewed and considered "done" before copying some of the same changes to quiz_question_bank_view.

            Show
            raymor Ray Morris added a comment - - edited After addressing my Git issues, the new revision is now available, rebased. 1332 / 1341 is the line I was seeking feedback on. I'd like to have this reviewed and considered "done" before copying some of the same changes to quiz_question_bank_view.
            Hide
            timhunt Tim Hunt added a comment -

            Sorry for not spotting this before, but question_bank_search_condition_array is a bad class name. It is not an array. It is a utily class that has a method that returns a list of coditions. I don't know what other methods you think that class may have. Have you provided a base class for these classes, to document the expected API?

            display_showtext_chk is not an appropriate name.

            Also, as I said above Every method should have "PHPdoc comments one blank line beteween methods."

            Look, I know you are getting frustrated that this is still not ready for integration, but I am getting frustrated that I keep having to point out the same mistakes. If you want to get your changes into Moodle it is your responsibility to get them right.

            To answer your question about display opitons: Well the method is protected, which means that it is fair game for subclasses to override this method. Hence it is part of the API of this class. We can only change the API if we think that gain is more than the pain. Has this every been overridden? Well, we don't know about add-ons, but we can check quiz_question_bank_view in core. Yes, that does override display_options, so you can't just change the API whithout doing something about backwards compatibility.

            Show
            timhunt Tim Hunt added a comment - Sorry for not spotting this before, but question_bank_search_condition_array is a bad class name. It is not an array. It is a utily class that has a method that returns a list of coditions. I don't know what other methods you think that class may have. Have you provided a base class for these classes, to document the expected API? display_showtext_chk is not an appropriate name. Also, as I said above Every method should have "PHPdoc comments one blank line beteween methods." Look, I know you are getting frustrated that this is still not ready for integration, but I am getting frustrated that I keep having to point out the same mistakes. If you want to get your changes into Moodle it is your responsibility to get them right. To answer your question about display opitons: Well the method is protected, which means that it is fair game for subclasses to override this method. Hence it is part of the API of this class. We can only change the API if we think that gain is more than the pain. Has this every been overridden? Well, we don't know about add-ons, but we can check quiz_question_bank_view in core. Yes, that does override display_options, so you can't just change the API whithout doing something about backwards compatibility.
            Hide
            raymor Ray Morris added a comment -

            I'm re-reading the naming conventions, along with the rest of http://docs.moodle.org/dev/Coding_style
            Sorry it's taking me some time to adjust from 17 years of doing it one way to switch to another way, the Moodle way + Tim way. (Consistency within the file vs. current Moodle standards).

            > question_bank_search_condition_array ... Have you provided a base class for these classes,
            > to document the expected API?

            Yes, question_bank_search_condition_array and question_bank_search_condition are abstract classes - they are the base classes. Let me know if that's not what you're referring to.

            I'm gone for the next three days if that affects merge planning and I'll submit another commit Monday or Tuesday, hopefully the final one for question/*. Let me make sure I'm understanding two things you said:

            > I don't know what other methods you think that class may have.

            Absolutely, I thought that was silly for it to be a class, but necessary due to my misunderstanding a Moodle API change. I'm thinking that should be a function, not a class, agreed?

            > display_showtext_chk is not an appropriate name.

            The very next method is "display_category_form_checkbox". "display_showtext_checkbox" to be consistent sound good?

            I'm sure it's frustrating for you. I'm trying to reduce that as much as possible. Most of this isn't too frustrating for me - you are pointing out things that should be fixed. Certainly restructuring is a lot more work than simply adding a few lines, but sometimes that's wise. The only thing that truly frustrates me is going back and rehashing decisions that have already been made explicitly or implicitly, turning tangential (and old) questions into blockers (ie Activity Completion isn't new, so how it ought to work shouldn't block new commits for months at a time).

            Show
            raymor Ray Morris added a comment - I'm re-reading the naming conventions, along with the rest of http://docs.moodle.org/dev/Coding_style Sorry it's taking me some time to adjust from 17 years of doing it one way to switch to another way, the Moodle way + Tim way. (Consistency within the file vs. current Moodle standards). > question_bank_search_condition_array ... Have you provided a base class for these classes, > to document the expected API? Yes, question_bank_search_condition_array and question_bank_search_condition are abstract classes - they are the base classes. Let me know if that's not what you're referring to. I'm gone for the next three days if that affects merge planning and I'll submit another commit Monday or Tuesday, hopefully the final one for question/*. Let me make sure I'm understanding two things you said: > I don't know what other methods you think that class may have. Absolutely, I thought that was silly for it to be a class, but necessary due to my misunderstanding a Moodle API change. I'm thinking that should be a function, not a class, agreed? > display_showtext_chk is not an appropriate name. The very next method is "display_category_form_checkbox". "display_showtext_checkbox" to be consistent sound good? I'm sure it's frustrating for you. I'm trying to reduce that as much as possible. Most of this isn't too frustrating for me - you are pointing out things that should be fixed. Certainly restructuring is a lot more work than simply adding a few lines, but sometimes that's wise. The only thing that truly frustrates me is going back and rehashing decisions that have already been made explicitly or implicitly, turning tangential (and old) questions into blockers (ie Activity Completion isn't new, so how it ought to work shouldn't block new commits for months at a time).
            Hide
            timhunt Tim Hunt added a comment -

            Yes, question_bank_search_condition_array as a function, perhaps called something like [local_whatever]_get_question_bank_search_conditions, sounds good.

            display_showtext_checkbox - yes.

            I hope you have a great few days away.

            Show
            timhunt Tim Hunt added a comment - Yes, question_bank_search_condition_array as a function, perhaps called something like [local_whatever] _get_question_bank_search_conditions, sounds good. display_showtext_checkbox - yes. I hope you have a great few days away.
            Hide
            raymor Ray Morris added a comment - - edited

            The two minor tweaks to the quiz_question_bank_view UI are as shown in the attached screenshots:

            Declutter - Checkboxes for "old questions" and "subcategories" are in a "show more" section.
            This can be very easily changed based on peer review.

            Consolidating the controls for which questions to show at the top of the form -
            Previously, the category selection was in the top section, and subcategory checkbox in the bottom section. This places the two category controls near each other.

            The consolidation is a side effect of making category selection "just another filter" - the controls for that filter are spatially together.

            Show
            raymor Ray Morris added a comment - - edited The two minor tweaks to the quiz_question_bank_view UI are as shown in the attached screenshots: Declutter - Checkboxes for "old questions" and "subcategories" are in a "show more" section. This can be very easily changed based on peer review. Consolidating the controls for which questions to show at the top of the form - Previously, the category selection was in the top section, and subcategory checkbox in the bottom section. This places the two category controls near each other. The consolidation is a side effect of making category selection "just another filter" - the controls for that filter are spatially together.
            Hide
            raymor Ray Morris added a comment - - edited

            The minimal sample plugin attached (local_searchbytags.zip) may be used to test the API.

            Show
            raymor Ray Morris added a comment - - edited The minimal sample plugin attached (local_searchbytags.zip) may be used to test the API.
            Hide
            raymor Ray Morris added a comment -

            I believe all concerns mentioned above have been addressed.

            Show
            raymor Ray Morris added a comment - I believe all concerns mentioned above have been addressed.
            Hide
            timhunt Tim Hunt added a comment -

            Looking at the screen-grabs, I think you need to do a bit more work on your CSS. For example why the huge gap above the category description? Why change the quiz question bank to centre some stuff? Overall the quiz question bank view now takes up more space vertically. Can you avoid that? The show question text option should not be there in the quiz question bank.

            Show
            timhunt Tim Hunt added a comment - Looking at the screen-grabs, I think you need to do a bit more work on your CSS. For example why the huge gap above the category description? Why change the quiz question bank to centre some stuff? Overall the quiz question bank view now takes up more space vertically. Can you avoid that? The show question text option should not be there in the quiz question bank.
            Hide
            raymor Ray Morris added a comment - - edited

            Done.

            I should have done another screen cap after I removed "show question text".

            > why the huge gap above the category description? Why change the quiz question bank
            > to centre some stuff?

            That was kind of yucky. That came from (mis)labeling it as a categorypicker class, which I've now removed.

            .categorypicker

            {text-align:center;margin-bottom:10px;}

            With that adjustment, the tweaked UI takes 12% less vertical space than the old, mostly because it doesn't repeat the current category name again.

            Show
            raymor Ray Morris added a comment - - edited Done. I should have done another screen cap after I removed "show question text". > why the huge gap above the category description? Why change the quiz question bank > to centre some stuff? That was kind of yucky. That came from (mis)labeling it as a categorypicker class, which I've now removed. .categorypicker {text-align:center;margin-bottom:10px;} With that adjustment, the tweaked UI takes 12% less vertical space than the old, mostly because it doesn't repeat the current category name again.
            Hide
            timhunt Tim Hunt added a comment -

            Well, having the categorypicker class on the select where you pick the category seems sensible to me. It makes it possible to apply particular styles there if you want.

            I expect the 10px bottom margin made a sensible visual grouping of the UI elements before you changed things.

            I still think your prosed new design is a small step backwards in terms of usability. You need to pay as much attention to the visual styling as was paid in the original.

            Show
            timhunt Tim Hunt added a comment - Well, having the categorypicker class on the select where you pick the category seems sensible to me. It makes it possible to apply particular styles there if you want. I expect the 10px bottom margin made a sensible visual grouping of the UI elements before you changed things. I still think your prosed new design is a small step backwards in terms of usability. You need to pay as much attention to the visual styling as was paid in the original.
            Hide
            raymor Ray Morris added a comment - - edited

            > having the categorypicker class on the select where you pick the category seems sensible to me

            It made sense to me too. Unfortunately, it's styled for it's original use on it's original page, picking course categories.

            > I expect the 10px bottom margin made a sensible visual grouping of the UI elements before you changed things.

            Please see the screenshots. There was no 10px bottom margin before I changed things. (And there's not now, either).

            > I still think your prosed new design is a small step backwards in terms of usability.

            It's almost identical, so being a math nerd, not a graphic designer, I don't have any ideas on how to improve it that aren't out of scope for this issue. Do you have any ideas?

            I think you've done an excellent job of picking out things that could be improved. You've been thorough to the point that some people might call it "nit picking", but there's a good reason to be thorough when picking nits - nits grow into lice.
            Please note we are now at the point where we're talking about me removing something that was never there, about something being bigger when it's actually smaller, etc. We may have found all the nits.

            Show
            raymor Ray Morris added a comment - - edited > having the categorypicker class on the select where you pick the category seems sensible to me It made sense to me too. Unfortunately, it's styled for it's original use on it's original page, picking course categories. > I expect the 10px bottom margin made a sensible visual grouping of the UI elements before you changed things. Please see the screenshots. There was no 10px bottom margin before I changed things. (And there's not now, either). > I still think your prosed new design is a small step backwards in terms of usability. It's almost identical, so being a math nerd, not a graphic designer, I don't have any ideas on how to improve it that aren't out of scope for this issue. Do you have any ideas? I think you've done an excellent job of picking out things that could be improved. You've been thorough to the point that some people might call it "nit picking", but there's a good reason to be thorough when picking nits - nits grow into lice. Please note we are now at the point where we're talking about me removing something that was never there, about something being bigger when it's actually smaller, etc. We may have found all the nits.
            Hide
            timhunt Tim Hunt added a comment -

            If this gets integrated when it is still not good enough, then I know I will be the one fixing the mess.

            Your goal is to change the back-end, and you have done a good job of that. However, you have also changed the front end. Now when implementing a back-end change, you have two options for what to do in the front-end.

            • Ensure the front end does not change at all as a result of the back-end change.
            • Change the front in to make it better.

            What you cannot do is

            • Change the front-end to make it worse.

            You may argue that judging the front-end is subjective. It is, partly. The judegments I care about are those of typical Moodle users. If that is not available, I will rely on the heuristics I know.

            Show
            timhunt Tim Hunt added a comment - If this gets integrated when it is still not good enough, then I know I will be the one fixing the mess. Your goal is to change the back-end, and you have done a good job of that. However, you have also changed the front end. Now when implementing a back-end change, you have two options for what to do in the front-end. Ensure the front end does not change at all as a result of the back-end change. Change the front in to make it better. What you cannot do is Change the front-end to make it worse. You may argue that judging the front-end is subjective. It is, partly. The judegments I care about are those of typical Moodle users. If that is not available, I will rely on the heuristics I know.
            Hide
            raymor Ray Morris added a comment - - edited

            Thanks for that comment. It's useful to make that explicit, although "it goes without saying" a reminder is always good. My goal was a marriage of the two "can do" - make changes as minimal as possible consistent with the functionality, with any changes being improvements, again subject to functionality constraints.

            Since I'm entirely left brained myself, I can only rely on those same heuristics or principles that you mentioned. However, my office is surrounded by right brained UI designers and graphic designers. I asked two of the professional designers to comment on the three adjustments that I made. I asked them if the change was an improvement and if they saw a better way of doing it. Their feedback was yes, yes, split.

            Both agreed that "this category and it's children" makes sense, so the subcategory checkbox should be near the category selector. Including me, that's three votes for that being an improvement.

            Both agreed that reducing space usage by not repeating the category name is an improvement.
            If you strongly disagree, it would be simple to add that back in.

            The designers were split as to Show more. With no plugins, there are two checkboxes beneath show more. One liked to reduce screen clutter as much as possible, the other thought that with only two elements the gain from Show more was minimal. Of course, if more options are added, either in core or via plugins, "Show more" would be more useful. Including myself, that's 2 votes for, 1 vote against. That's also trivial to remove if you prefer.

            Neither designer had ideas on how to improve the UI related to this issue.
            A redesign of the whole UI, outside the scope of this ticket, is of course possible for the future. I have the perfect person here to help, an oddball genius who is both artist and programmer. If and when she has time to help with that she'd be very good at it.

            Do you have any specific thoughts for improvements or adjustments to any of the three tweaks I've made? Between you, the two designers, and myself, that's four people looking at it.

            Show
            raymor Ray Morris added a comment - - edited Thanks for that comment. It's useful to make that explicit, although "it goes without saying" a reminder is always good. My goal was a marriage of the two "can do" - make changes as minimal as possible consistent with the functionality, with any changes being improvements, again subject to functionality constraints. Since I'm entirely left brained myself, I can only rely on those same heuristics or principles that you mentioned. However, my office is surrounded by right brained UI designers and graphic designers. I asked two of the professional designers to comment on the three adjustments that I made. I asked them if the change was an improvement and if they saw a better way of doing it. Their feedback was yes, yes, split. Both agreed that "this category and it's children" makes sense, so the subcategory checkbox should be near the category selector. Including me, that's three votes for that being an improvement. Both agreed that reducing space usage by not repeating the category name is an improvement. If you strongly disagree, it would be simple to add that back in. The designers were split as to Show more. With no plugins, there are two checkboxes beneath show more. One liked to reduce screen clutter as much as possible, the other thought that with only two elements the gain from Show more was minimal. Of course, if more options are added, either in core or via plugins, "Show more" would be more useful. Including myself, that's 2 votes for, 1 vote against. That's also trivial to remove if you prefer. Neither designer had ideas on how to improve the UI related to this issue. A redesign of the whole UI, outside the scope of this ticket, is of course possible for the future. I have the perfect person here to help, an oddball genius who is both artist and programmer. If and when she has time to help with that she'd be very good at it. Do you have any specific thoughts for improvements or adjustments to any of the three tweaks I've made? Between you, the two designers, and myself, that's four people looking at it.
            Hide
            timhunt Tim Hunt added a comment -

            Well, I was going to make a patch to show you how easy it was to improve your UI, but having pulled your latest code, the JavaScript does not work.

            Note that Moodle should work without JavaScript. At the moment, with JavaScript off, users get a useless Show more... link.

            Also, I remain unconvinced that we should be using a whole lot of custom JavaScript here. (Well, I am certain I don't want to have to maintain that in future). I still thing using print_collapsible_region is a better option.

            Show
            timhunt Tim Hunt added a comment - Well, I was going to make a patch to show you how easy it was to improve your UI, but having pulled your latest code, the JavaScript does not work. Note that Moodle should work without JavaScript. At the moment, with JavaScript off, users get a useless Show more... link. Also, I remain unconvinced that we should be using a whole lot of custom JavaScript here. (Well, I am certain I don't want to have to maintain that in future). I still thing using print_collapsible_region is a better option.
            Hide
            raymor Ray Morris added a comment -

            It has been updated to use print_collapsible_region. Sorry about the Javascript, I can't directly test the Javascript
            produced by shifter due to internal policies.

            (The dev Moodle I test with can't be connected to the network, and therefore can neither use git nor install shifter.
            Yes, that's silly, and I'm working on getting a slightly less silly development environment approved.)

            Show
            raymor Ray Morris added a comment - It has been updated to use print_collapsible_region. Sorry about the Javascript, I can't directly test the Javascript produced by shifter due to internal policies. (The dev Moodle I test with can't be connected to the network, and therefore can neither use git nor install shifter. Yes, that's silly, and I'm working on getting a slightly less silly development environment approved.)
            Hide
            timhunt Tim Hunt added a comment -

            So close, but still not there. I am trying to understand the changes you have made to the HTML, and it makes no sense.

            1. What is <input type="hidden" id="showadv" value="0" name="showadv" /> for in the HTML?

            2. In the past, the quiz question bank showed the category info shortened to at most 200 characters and stripped down to plain text, while the general question bank showed the full thing. This was sensible given the different amount of space in each place. In your desire to use the same code in both places, you have removed the special handling on the quiz edit page. This will be a disaster if a category has a long description. I cannot immidiately see an easy fix for that.

            3. Similarly, there is no need to change the grey background behind the category description, which came from <div class="categoryinfo">.

            4. In the quiz, in the past, the settings at the top of the question bank were on a pale blue background like the create new question button, and the other controls at the bottom. Now the backgorund is white. Why? I think the form needs to be inside <div class="box generalbox questionbank">.

            Some other points:

            5. You deprecated the display_options method, but you are still calling it in quiz_question_bank_view.

            6. Do we really need a custom YUI module for this. I note that moodle-core-formautosubmit already exists. (It is used by the standard $OUTPUT->single_select()). Not sure if it copes with checkboxes. Sadly it doesn't. However, in the long term it would be easier to maintain an improved lib/yui/src/formautosubmit/js/formautosubmit.js than our own code.

            In my attempt to clean up the remaining problems, I created https://github.com/timhunt/moodle/commit/d706501fe2c9d161934d903a286553831bc741bc, which addresses some, but not all of the above problems, so I don't know how much use it is.

            The reason it is importnat to minimise the changes to the HTML is that every change to the HTML potentially breaks peoples custom themes.

            Anyway, I am afraid we are not there yet. I was hoping we coudl get this into integration next week

            Show
            timhunt Tim Hunt added a comment - So close, but still not there. I am trying to understand the changes you have made to the HTML, and it makes no sense. 1. What is <input type="hidden" id="showadv" value="0" name="showadv" /> for in the HTML? 2. In the past, the quiz question bank showed the category info shortened to at most 200 characters and stripped down to plain text, while the general question bank showed the full thing. This was sensible given the different amount of space in each place. In your desire to use the same code in both places, you have removed the special handling on the quiz edit page. This will be a disaster if a category has a long description. I cannot immidiately see an easy fix for that. 3. Similarly, there is no need to change the grey background behind the category description, which came from <div class="categoryinfo">. 4. In the quiz, in the past, the settings at the top of the question bank were on a pale blue background like the create new question button, and the other controls at the bottom. Now the backgorund is white. Why? I think the form needs to be inside <div class="box generalbox questionbank">. Some other points: 5. You deprecated the display_options method, but you are still calling it in quiz_question_bank_view. 6. Do we really need a custom YUI module for this. I note that moodle-core-formautosubmit already exists. (It is used by the standard $OUTPUT->single_select()). Not sure if it copes with checkboxes. Sadly it doesn't. However, in the long term it would be easier to maintain an improved lib/yui/src/formautosubmit/js/formautosubmit.js than our own code. In my attempt to clean up the remaining problems, I created https://github.com/timhunt/moodle/commit/d706501fe2c9d161934d903a286553831bc741bc , which addresses some, but not all of the above problems, so I don't know how much use it is. The reason it is importnat to minimise the changes to the HTML is that every change to the HTML potentially breaks peoples custom themes. Anyway, I am afraid we are not there yet. I was hoping we coudl get this into integration next week
            Hide
            raymor Ray Morris added a comment - - edited

            I really appreciate your comments and especially your commit.

            #1. Brain fart. It's purpose was filled by collapsible_region.

            #2. It's unfortunate that text-overflow: ellipsis requires white-space: nowrap.
            I'm attaching three screenshots of a category with a very long description.
            The first is how it appears if we do nothing. The second sets minimal CSS to limit it to
            approximately 200 characters (specifically, three lines). The third uses trickier CSS
            to give a fade out effect. Other than what's shown -there, I don't see another elegant
            solution either.

            #6. I'll open an issue on formautosubmit/js/formautosubmit.js and get to work on that.
            In case improvingformautosubmit/js/formautosubmit.js spawns another issue or for some
            other reason takes takes six months, how about we don't make that a blocker here.
            When formautosubmit.js can handle it, I'll git rm moodle-question-searchform.js.

            > I was hoping we coudl get this into integration next week

            I believe we've addressed everything, depending on what decision is made on #2 and #6.

            Show
            raymor Ray Morris added a comment - - edited I really appreciate your comments and especially your commit. #1. Brain fart. It's purpose was filled by collapsible_region. #2. It's unfortunate that text-overflow: ellipsis requires white-space: nowrap. I'm attaching three screenshots of a category with a very long description. The first is how it appears if we do nothing. The second sets minimal CSS to limit it to approximately 200 characters (specifically, three lines). The third uses trickier CSS to give a fade out effect. Other than what's shown -there, I don't see another elegant solution either . #6. I'll open an issue on formautosubmit/js/formautosubmit.js and get to work on that. In case improvingformautosubmit/js/formautosubmit.js spawns another issue or for some other reason takes takes six months, how about we don't make that a blocker here. When formautosubmit.js can handle it, I'll git rm moodle-question-searchform.js. > I was hoping we coudl get this into integration next week I believe we've addressed everything, depending on what decision is made on #2 and #6.
            Hide
            raymor Ray Morris added a comment -

            After leaving work for the weekend, it occurred to me that I can probably make the category description look exactly like it did before.
            I can add the ellipses using the type of CSS that I used for the fade out effect. Because the development system isn't network accessible,
            I can't actually test it until Monday.

            Show
            raymor Ray Morris added a comment - After leaving work for the weekend, it occurred to me that I can probably make the category description look exactly like it did before. I can add the ellipses using the type of CSS that I used for the fade out effect. Because the development system isn't network accessible, I can't actually test it until Monday.
            Hide
            raymor Ray Morris added a comment -

            Ellipsis doesn't pad right. The ellipsis can be included, but it won't take any right padding, yet. Future browsers may solve this will an extension to txt-overflow: ellipsis.

            Show
            raymor Ray Morris added a comment - Ellipsis doesn't pad right. The ellipsis can be included, but it won't take any right padding, yet. Future browsers may solve this will an extension to txt-overflow: ellipsis.
            Hide
            timhunt Tim Hunt added a comment -

            Well, testing this was productive. Two unrelated bugs found: MDL-41035, MDL-41036

            Trying to fix MDL-41035 made me realise quite how scrambled this code is, both before and after your fix. (You should have seen it before it introduced the question_bank_view class. It was worse still.)

            We need to do something about categories with long descriptions. I suggest

            1. adding a $maxinfolength = null argument to print_category_info and display_options
            2. passing 200 there in the quiz code.
            3. calling shorten_text in print_category_info, just like it used to.

            Note that once everthing is done, and this is rebased down to a single commit, you need to fix the issue number in the commit comment. However, please do not rebase to a single commit until everything is fixed, so I can revew just the most recent changes you make.

            Show
            timhunt Tim Hunt added a comment - Well, testing this was productive. Two unrelated bugs found: MDL-41035 , MDL-41036 Trying to fix MDL-41035 made me realise quite how scrambled this code is, both before and after your fix. (You should have seen it before it introduced the question_bank_view class. It was worse still.) We need to do something about categories with long descriptions. I suggest adding a $maxinfolength = null argument to print_category_info and display_options passing 200 there in the quiz code. calling shorten_text in print_category_info, just like it used to. Note that once everthing is done, and this is rebased down to a single commit, you need to fix the issue number in the commit comment. However, please do not rebase to a single commit until everything is fixed, so I can revew just the most recent changes you make.
            Hide
            raymor Ray Morris added a comment -

            $maxinfolength has been added as suggested. I passed it at the same point all other arguments are passed.

            > Trying to fix MDL-41035 made me realise quite how scrambled this code is, both before and after your fix.

            That's a relief - It's not that I've become dense in my old age after all.
            I shall endeavour to reduce the scramble quotient in the process of any other alterations.

            Show
            raymor Ray Morris added a comment - $maxinfolength has been added as suggested. I passed it at the same point all other arguments are passed. > Trying to fix MDL-41035 made me realise quite how scrambled this code is, both before and after your fix. That's a relief - It's not that I've become dense in my old age after all. I shall endeavour to reduce the scramble quotient in the process of any other alterations.
            Hide
            timhunt Tim Hunt added a comment -

            Don't add the extra argument in the constructor. There is no need for it to be part of the state. It is only needed on output.

            Also, you can't call shorten_text will null max length.

            Show
            timhunt Tim Hunt added a comment - Don't add the extra argument in the constructor. There is no need for it to be part of the state. It is only needed on output. Also, you can't call shorten_text will null max length.
            Hide
            raymor Ray Morris added a comment -

            Something seems wrong about passing, to every plugin, the maximum length you want for the CATEGORY description to be. It seems to me that the length of the category description should be a property of the category or it's plugin only. It's only relevant to that one plugin.

            Since it's purely a presentational issue, I tend to do favor handling it in the CSS.

            If you're certain it makes sense to pass it to every plugin in the display loop, I'll do it that way. Even though I think it's a property of the category display, I'll defer if you insist.

            Show
            raymor Ray Morris added a comment - Something seems wrong about passing, to every plugin, the maximum length you want for the CATEGORY description to be. It seems to me that the length of the category description should be a property of the category or it's plugin only. It's only relevant to that one plugin. Since it's purely a presentational issue, I tend to do favor handling it in the CSS. If you're certain it makes sense to pass it to every plugin in the display loop, I'll do it that way. Even though I think it's a property of the category display, I'll defer if you insist.
            Hide
            raymor Ray Morris added a comment -

            Come to think of it, I can think of two other options. Since we don't know, in general, what a plugin might like to know, it currently passes $this. Therefore, without changing any of the calling code, the category filter could make use of $caller:MAX_TEXT_LENGTH.
            That wouldn't alter the clean calling semantics.

            Alternatively, one could also change this:

            foreach ($this->searchconditions as $searchcondition) {
                echo $searchcondition->display_options($this);
            }

            To this:

            $maxlength = null;
            foreach ($this->searchconditions as $searchcondition) {
                if (get_class($searchcondition) == 'question_bank_search_condition_category') {
                    $maxlength = self::MAX_TEXT_LENGTH;
                ]
                echo $searchcondition->display_options($this, $maxlength);
            }

            That seems less clean.

            Show
            raymor Ray Morris added a comment - Come to think of it, I can think of two other options. Since we don't know, in general, what a plugin might like to know, it currently passes $this. Therefore, without changing any of the calling code, the category filter could make use of $caller:MAX_TEXT_LENGTH. That wouldn't alter the clean calling semantics. Alternatively, one could also change this: foreach ($this->searchconditions as $searchcondition) { echo $searchcondition->display_options($this); } To this: $maxlength = null; foreach ($this->searchconditions as $searchcondition) { if (get_class($searchcondition) == 'question_bank_search_condition_category') { $maxlength = self::MAX_TEXT_LENGTH; ] echo $searchcondition->display_options($this, $maxlength); } That seems less clean.
            Hide
            timhunt Tim Hunt added a comment -

            OK, so it is a mess. The main thing this code needs is separating out the display code into a renderer, but we are not going to do that now.

            So, I guess passing it in the constructor is the least-bad option.

            You still need to change it to only call shorten_text if max length is not null.

            Also, you should not call strip tags.

            Finally, I have been wondering if 'Search options' would be a better string than 'Advanced options'.

            Finally, we need to rebase this down to one commit.

            Show
            timhunt Tim Hunt added a comment - OK, so it is a mess. The main thing this code needs is separating out the display code into a renderer, but we are not going to do that now. So, I guess passing it in the constructor is the least-bad option. You still need to change it to only call shorten_text if max length is not null. Also, you should not call strip tags. Finally, I have been wondering if 'Search options' would be a better string than 'Advanced options'. Finally, we need to rebase this down to one commit.
            Hide
            raymor Ray Morris added a comment - - edited

            Done: if max length is not null.
            Done: not call strip tags.
            Done: 'Search options' would be a better string than 'Advanced options'.

            Would you care to take a quick look at that short commit before I rebase?
            I expect I might have difficulty rebasing this time due to the intervening commit for MDL-35053 vs. my inexperience with rebasing with git, especially after pushing and after pulling your commit, so I'm hoping to only do that once now that there is an intervening commit.

            Show
            raymor Ray Morris added a comment - - edited Done: if max length is not null. Done: not call strip tags. Done: 'Search options' would be a better string than 'Advanced options'. Would you care to take a quick look at that short commit before I rebase? I expect I might have difficulty rebasing this time due to the intervening commit for MDL-35053 vs. my inexperience with rebasing with git, especially after pushing and after pulling your commit, so I'm hoping to only do that once now that there is an intervening commit.
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, I went to bed last night just before your last comment.

            That last commit is OK. (I would not have written the if with so much duplicated code.) Go for rebase.

            Show
            timhunt Tim Hunt added a comment - Sorry, I went to bed last night just before your last comment. That last commit is OK. (I would not have written the if with so much duplicated code.) Go for rebase.
            Hide
            timhunt Tim Hunt added a comment -

            Since you say you don't have much experience with rebase, some tips:

            First remember to pull lastest master from moodle.git, and push it to your github.

            1. The classic way to do it would be

            git rebase -i master

            which should let convert everything to a single commit if you squash everything after the first commit. (Don't worry about keeping my commit separate if you are happy to commit it all under your own name.) You will probably have to sort out merge conflicts along the way.

            2. An alternative, since we are shooting for a single commit is:

            git checkout master
            git merge --squash MDL-4013-question_filtering_api_f
            git commit

            In either case, make sure you write a good commit comment.

            Show
            timhunt Tim Hunt added a comment - Since you say you don't have much experience with rebase, some tips: First remember to pull lastest master from moodle.git, and push it to your github. 1. The classic way to do it would be git rebase -i master which should let convert everything to a single commit if you squash everything after the first commit. (Don't worry about keeping my commit separate if you are happy to commit it all under your own name.) You will probably have to sort out merge conflicts along the way. 2. An alternative, since we are shooting for a single commit is: git checkout master git merge --squash MDL-4013 -question_filtering_api_f git commit In either case, make sure you write a good commit comment.
            Hide
            raymor Ray Morris added a comment -

            The rebased commit has been pushed to:
            https://github.com/MorrisR2/moodle/commit/d9080f6745f2f783a4dd768feda309110e09cd36

            Thanks for the hints in your last message, Tim.

            Show
            raymor Ray Morris added a comment - The rebased commit has been pushed to: https://github.com/MorrisR2/moodle/commit/d9080f6745f2f783a4dd768feda309110e09cd36 Thanks for the hints in your last message, Tim.
            Hide
            timhunt Tim Hunt added a comment -
            Show
            timhunt Tim Hunt added a comment - Yay! We got there. Submitting for integration now. (By the way, this comment may amuse you: https://tracker.moodle.org/browse/MDL-31226?focusedCommentId=236882&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-236882 )
            Hide
            raymor Ray Morris added a comment -

            Thanks Tim. very, very rarely do I actually LOL, but that time I did.

            Show
            raymor Ray Morris added a comment - Thanks Tim. very, very rarely do I actually LOL, but that time I did.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Ray,

            I'm afraid I can't find a clean branch rebased on top of moodle master to integrate this change.

            I'm going to proceed thinking that cherry-picking d9080f6745f2f783a4dd768feda309110e09cd36 will be sufficient and see how far I get.

            In future, it'd be great if you can provide a branch which we can merge cleanly.
            thanks,
            Dan

            Show
            poltawski Dan Poltawski added a comment - Hi Ray, I'm afraid I can't find a clean branch rebased on top of moodle master to integrate this change. I'm going to proceed thinking that cherry-picking d9080f6745f2f783a4dd768feda309110e09cd36 will be sufficient and see how far I get. In future, it'd be great if you can provide a branch which we can merge cleanly. thanks, Dan
            Hide
            poltawski Dan Poltawski added a comment -

            Sorry - I did not get very far - it looks like lots of useful information in question/upgrade.txt is being removed by this patch. I think the intention is to add new information there not remove the existing 2.6 info and so i'm going to reopen this for now.

            There are also problems with trailing whitespace which I would've fixed up, but if they could be sorted out it would be good too.

            Otherwise it looks fine to me - careful with the spacing on this line:

            $go = html_writer::empty_tag('input', array('type'=>'submit', 'value'=>get_string('go')));
            

            thanks,
            dan

            Show
            poltawski Dan Poltawski added a comment - Sorry - I did not get very far - it looks like lots of useful information in question/upgrade.txt is being removed by this patch. I think the intention is to add new information there not remove the existing 2.6 info and so i'm going to reopen this for now. There are also problems with trailing whitespace which I would've fixed up, but if they could be sorted out it would be good too. Otherwise it looks fine to me - careful with the spacing on this line: $go = html_writer::empty_tag('input', array('type'=>'submit', 'value'=>get_string('go'))); thanks, dan
            Hide
            poltawski Dan Poltawski added a comment -

            Oh, one final comment - in the commit your name is MorrisR2 - perhaps you'd like to use your real name there

            Show
            poltawski Dan Poltawski added a comment - Oh, one final comment - in the commit your name is MorrisR2 - perhaps you'd like to use your real name there
            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 -

            Those issues have been addressed.
            I have rebased the branch, including upgrade.txt. TheURL has been updated to the newly rebased branch.
            I have corrected the trailing whitespace on continuation lines, and updated my name.

            I'm not sure what Dan is referring to regarding the html_writer::empty_tag line.
            I've carefully gone through each point in http://docs.moodle.org/dev/Coding_style and I'm not seeing the problem.

            Show
            raymor Ray Morris added a comment - Those issues have been addressed. I have rebased the branch, including upgrade.txt. TheURL has been updated to the newly rebased branch. I have corrected the trailing whitespace on continuation lines, and updated my name. I'm not sure what Dan is referring to regarding the html_writer::empty_tag line. I've carefully gone through each point in http://docs.moodle.org/dev/Coding_style and I'm not seeing the problem.
            Hide
            timhunt Tim Hunt added a comment -

            Ray, sorry for the lack of response form us. That new branch looks OK. Can you want for this weekly to be released in a day or two, then rebase again down to a single commit (which should be quick and easy) then I will submit this for interation again.

            Thanks.

            Show
            timhunt Tim Hunt added a comment - Ray, sorry for the lack of response form us. That new branch looks OK. Can you want for this weekly to be released in a day or two, then rebase again down to a single commit (which should be quick and easy) then I will submit this for interation again. Thanks.
            Hide
            raymor Ray Morris added a comment -
            Show
            raymor Ray Morris added a comment - This has been rebased to weekly. https://github.com/MorrisR2/moodle/compare/MOODLE_25_STABLE
            Hide
            timhunt Tim Hunt added a comment -

            A new feature like this should be rebased onto the moodle.org master branch, not a stable branch.

            Show
            timhunt Tim Hunt added a comment - A new feature like this should be rebased onto the moodle.org master branch, not a stable branch.
            Hide
            raymor Ray Morris added a comment -

            That's what I had thought, until you said " Can you want for this weekly to be released in a day or two, then rebase again ", so I rebased against the weekly. Updating to master.

            Show
            raymor Ray Morris added a comment - That's what I had thought, until you said " Can you want for this weekly to be released in a day or two, then rebase again ", so I rebased against the weekly. Updating to master.
            Hide
            timhunt Tim Hunt added a comment -

            There is a weekly release for each branch, including master. Sorry for the confusion.

            Show
            timhunt Tim Hunt added a comment - There is a weekly release for each branch, including master. Sorry for the confusion.
            Hide
            raymor Ray Morris added a comment -
            Show
            raymor Ray Morris added a comment - Thanks for the clarification. https://github.com/MorrisR2/moodle/compare/moodle:master...master
            Hide
            timhunt Tim Hunt added a comment -

            Yay! Thanks Ray. Sumitting for integration.

            Show
            timhunt Tim Hunt added a comment - Yay! Thanks Ray. Sumitting for integration.
            Hide
            marina Marina Glancy added a comment -

            Hi, thanks guys for working on it.
            This issue needs a little more work:
            1. Deprecation: http://docs.moodle.org/dev/Deprecation

            • use correct phpdocs:
              @deprecated since 2.6
              @see new_function_name()
            • Inside function you need to call debugging()
            • You need to convert all current usages of the deprecated functions
            • New issue should be created to remove those functions in 2.7 (in META MDL-35024)

            2. Unittests - really-really needed here

            3. init_search_conditions() - you look for method but create a class. If you had unittests you would test this.
            Time to use autoloader - see core_component::get_plugin_list_with_class()
            Petr Skoda says that it is not expensive to add the similar function that would look for class in ANY plugin type. Which would be a good idea here - why limit only to local plugins?

            4. Again, consider placing new classes in /question/classes/ folder and use namespace \core_question\ or at least start the name of the class with 'core_question_'
            http://docs.moodle.org/dev/Automatic_class_loading

            Show
            marina Marina Glancy added a comment - Hi, thanks guys for working on it. This issue needs a little more work: 1. Deprecation: http://docs.moodle.org/dev/Deprecation use correct phpdocs: @deprecated since 2.6 @see new_function_name() Inside function you need to call debugging() You need to convert all current usages of the deprecated functions New issue should be created to remove those functions in 2.7 (in META MDL-35024 ) 2. Unittests - really-really needed here 3. init_search_conditions() - you look for method but create a class. If you had unittests you would test this. Time to use autoloader - see core_component::get_plugin_list_with_class() Petr Skoda says that it is not expensive to add the similar function that would look for class in ANY plugin type. Which would be a good idea here - why limit only to local plugins? 4. Again, consider placing new classes in /question/classes/ folder and use namespace \core_question\ or at least start the name of the class with 'core_question_' http://docs.moodle.org/dev/Automatic_class_loading
            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 - - edited

            > you look for method but create a class. If you had unittests you would test this.

            It calls a function which returns a list of search classes. See the comment above https://tracker.moodle.org/browse/MDL-40313?focusedCommentId=234284&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-234284 . A plugin can add more than one condition. Based on it's settings, it could even return zero conditions.

            Show
            raymor Ray Morris added a comment - - edited > you look for method but create a class. If you had unittests you would test this. It calls a function which returns a list of search classes. See the comment above https://tracker.moodle.org/browse/MDL-40313?focusedCommentId=234284&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-234284 . A plugin can add more than one condition. Based on it's settings, it could even return zero conditions.
            Hide
            raymor Ray Morris added a comment - - edited

            > see core_component::get_plugin_list_with_class()
            > Petr Škoda says that it is not expensive to add the similar function that would look for class in ANY plugin type.

            Two questions:

            1) What's the advantage to get_plugin_list_with_class versus get_plugin_list_with_function? Why wrap a single function into a class? Autoloading of returned classes is not affected. Tim and I agree that's a bad idea (see above discussion).

            2) to add the similar function that would look for class in ANY plugin type

            "The similar function" meaning which function, please? All of the get_plugin* functions I see take the type of plugin as the first argument. Is he talking about writing some new plugin API with functions like get_plugin_list_*_anywhere"? That sounds like a good idea, but is rewriting the plugin API not WAY outside the scope of this issue?

            Show
            raymor Ray Morris added a comment - - edited > see core_component::get_plugin_list_with_class() > Petr Škoda says that it is not expensive to add the similar function that would look for class in ANY plugin type. Two questions: 1) What's the advantage to get_plugin_list_with_class versus get_plugin_list_with_function? Why wrap a single function into a class? Autoloading of returned classes is not affected. Tim and I agree that's a bad idea (see above discussion). 2) to add the similar function that would look for class in ANY plugin type "The similar function" meaning which function, please? All of the get_plugin* functions I see take the type of plugin as the first argument. Is he talking about writing some new plugin API with functions like get_plugin_list_*_anywhere"? That sounds like a good idea, but is rewriting the plugin API not WAY outside the scope of this issue?
            Hide
            marina Marina Glancy added a comment -

            Hi Ray,

            Sorry, did not realise that the function returns list of classes. So there is no error in code here but isn't it overcomplicating? At least the function can return the list of instances of the classes and not their names.

            1. core_component::get_plugin_list_with_class() looks through the cached list of existing classes.
            get_plugin_list_with_function() loads the lib.php file in every plugin of the specified type and search for function using php method.
            The performance difference is significant.

            But now after your explanation (that there could be multiple classes in one plugin) I'm not so sure about it. As much as I would like to move everything from searching for functions towards autoloading classes, here it might be not that convenient.

            2. by "similar function" I meant core_component::get_plugin_list_with_class(). Since it does not include any files and basically looks through a static array, it is not expensive to search in all plugin types.
            It's not rewriting API, it's just a small modification to this function to allow to submit 'null' instead of plugintype.
            This function is only being introduced in 2.6.
            Obviously you can't call get_plugin_list_with_function() for all plugin types because it would be extremely expensive.

            Show
            marina Marina Glancy added a comment - Hi Ray, Sorry, did not realise that the function returns list of classes. So there is no error in code here but isn't it overcomplicating? At least the function can return the list of instances of the classes and not their names. 1. core_component::get_plugin_list_with_class() looks through the cached list of existing classes. get_plugin_list_with_function() loads the lib.php file in every plugin of the specified type and search for function using php method. The performance difference is significant. But now after your explanation (that there could be multiple classes in one plugin) I'm not so sure about it. As much as I would like to move everything from searching for functions towards autoloading classes, here it might be not that convenient. 2. by "similar function" I meant core_component::get_plugin_list_with_class(). Since it does not include any files and basically looks through a static array, it is not expensive to search in all plugin types. It's not rewriting API, it's just a small modification to this function to allow to submit 'null' instead of plugintype. This function is only being introduced in 2.6. Obviously you can't call get_plugin_list_with_function() for all plugin types because it would be extremely expensive.
            Hide
            raymor Ray Morris added a comment -

            Marina, thank you for your comments. Regarding "At least the function can return the list of instances of the classes and not their names", I agree, but I don't think Tim does. Hopefully I'm wrong.

            In MDL-40457, I had the plugins return instances, rather than their names. Tim asked that I return the names so that question bank can force autoloading. If the plugin returns a instance, it may have autoloaded that instance, or it may not have. In MY opinion, it's better to return the instance because as I mentioned in MDL-40457a plugin may need to return a non-autoloaded class such as question_bank_question_name_text_column. So we're both in agreement, but I'd like to give $maintainer a chance to comment before making the change.

            Show
            raymor Ray Morris added a comment - Marina, thank you for your comments. Regarding "At least the function can return the list of instances of the classes and not their names", I agree, but I don't think Tim does. Hopefully I'm wrong. In MDL-40457 , I had the plugins return instances, rather than their names. Tim asked that I return the names so that question bank can force autoloading. If the plugin returns a instance, it may have autoloaded that instance, or it may not have. In MY opinion, it's better to return the instance because as I mentioned in MDL-40457 a plugin may need to return a non-autoloaded class such as question_bank_question_name_text_column. So we're both in agreement, but I'd like to give $maintainer a chance to comment before making the change.
            Hide
            timhunt Tim Hunt added a comment -

            The difference between question bank columns and these search conditions is that:

            • For column types, the local plugin can return a list of columnt types that it supports, but they may or many not acutally be used depending on a configuration settings. Hence returning instances would be inefficient.
            • The search conditions are always used, so returning instances makes more sense.

            So, it would be good if you could change it.

            Show
            timhunt Tim Hunt added a comment - The difference between question bank columns and these search conditions is that: For column types, the local plugin can return a list of columnt types that it supports, but they may or many not acutally be used depending on a configuration settings. Hence returning instances would be inefficient. The search conditions are always used, so returning instances makes more sense. So, it would be good if you could change it.
            Hide
            raymor Ray Morris added a comment -

            @Marina, the sole remaining blocker is unit tests. (Everything else mentioned has been addressed in git and I can post details as needed.) Question bank is not, and to my knowledge never has been, testable.
            To make question bank work with our unit testing frameworks will require significant redesign. Along with the typical process of changing private methods to be public, adding new testable methods, etc., some of the HTML rendering needs to be significantly rewritten, I believe.

            Certainly, these things should be done. However, I propose that a significant rewrite of question bank to make it testable, and then writing a suite of tests, is outside the scope of this particular issue. Certainly, it's become apparent that I, the author of this item, can't be the one to do that because many of the necessary decisions rightfully belong to Tim. There are architectural changes required and those are very much in Tim's area of authority and responsibility.

            I've posted testing instructions and one test plugin. I can post some other test plugins that we use on a daily basis. What else can we do to move this forward?

            Show
            raymor Ray Morris added a comment - @Marina, the sole remaining blocker is unit tests. (Everything else mentioned has been addressed in git and I can post details as needed.) Question bank is not, and to my knowledge never has been, testable. To make question bank work with our unit testing frameworks will require significant redesign. Along with the typical process of changing private methods to be public, adding new testable methods, etc., some of the HTML rendering needs to be significantly rewritten, I believe. Certainly, these things should be done. However, I propose that a significant rewrite of question bank to make it testable, and then writing a suite of tests, is outside the scope of this particular issue. Certainly, it's become apparent that I, the author of this item, can't be the one to do that because many of the necessary decisions rightfully belong to Tim. There are architectural changes required and those are very much in Tim's area of authority and responsibility. I've posted testing instructions and one test plugin. I can post some other test plugins that we use on a daily basis. What else can we do to move this forward?
            Hide
            timhunt Tim Hunt added a comment -

            I agree with Ray, it is not fair to ask Ray to write the first unit tests for some code where I should have written tests long ago.

            Show
            timhunt Tim Hunt added a comment - I agree with Ray, it is not fair to ask Ray to write the first unit tests for some code where I should have written tests long ago.
            Hide
            marina Marina Glancy added a comment -

            Hello Ray, Tim.
            Sorry for the slow reply, I have just come back from vacation. I can not review the code right now because I have too much mail waiting for reply. Feel free to submit it for integration review now. If creating unittests is too complicated we won't push this requirement. Thanks again.
            Marina

            Show
            marina Marina Glancy added a comment - Hello Ray, Tim. Sorry for the slow reply, I have just come back from vacation. I can not review the code right now because I have too much mail waiting for reply. Feel free to submit it for integration review now. If creating unittests is too complicated we won't push this requirement. Thanks again. Marina
            Hide
            timhunt Tim Hunt added a comment -

            Ray, can you rebase the code one more time, then I will re-submit it for integration. Thanks.

            Show
            timhunt Tim Hunt added a comment - Ray, can you rebase the code one more time, then I will re-submit it for integration. Thanks.
            Hide
            raymor Ray Morris added a comment -

            it has been rebased at https://github.com/MorrisR2/moodle/compare/moodle:master...master

            Thanks Tim, Marina.

            Show
            raymor Ray Morris added a comment - it has been rebased at https://github.com/MorrisR2/moodle/compare/moodle:master...master Thanks Tim, Marina.
            Hide
            timhunt Tim Hunt added a comment -

            I'll look at this properly tomorrow.

            Show
            timhunt Tim Hunt added a comment - I'll look at this properly tomorrow.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40313

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40313 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/493 Warning: The master branch at git://github.com/MorrisR2/moodle.git has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/493/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, Ray, I just spotted some things ...

            Acutally, I am going to take a crack at fixing these myself: https://github.com/timhunt/moodle/compare/master...MDL-40313

            1. The "@todo MDL-41978 This will be deleted in Moodle 2.7" comments. These now need to say 2.8 - oh, I see. You only missed one. I have fixed this by amending your commit.

            2. Codechecker issues that CiBoT found above. Done.

            3. Also, it would be nice to namespace the classes better. Also done.

            4. Just spotted that some files are 755, not 644. Will fix soon ...

            possibly more to come.

            Show
            timhunt Tim Hunt added a comment - Sorry, Ray, I just spotted some things ... Acutally, I am going to take a crack at fixing these myself: https://github.com/timhunt/moodle/compare/master...MDL-40313 1. The "@todo MDL-41978 This will be deleted in Moodle 2.7" comments. These now need to say 2.8 - oh, I see. You only missed one. I have fixed this by amending your commit. 2. Codechecker issues that CiBoT found above. Done. 3. Also, it would be nice to namespace the classes better. Also done. 4. Just spotted that some files are 755, not 644. Will fix soon ... possibly more to come.
            Hide
            raymor Ray Morris added a comment -

            Thanks Tim. My workflow is still a little broken so it's easy for me to get little snafus like wrong permissions.

            Show
            raymor Ray Morris added a comment - Thanks Tim. My workflow is still a little broken so it's easy for me to get little snafus like wrong permissions.
            Hide
            timhunt Tim Hunt added a comment -

            OK. I think I am doing screwing around with this.

            Ray, are you happy with the way I have messed around with your code?

            Show
            timhunt Tim Hunt added a comment - OK. I think I am doing screwing around with this. Ray, are you happy with the way I have messed around with your code?
            Hide
            timhunt Tim Hunt added a comment -

            Note, unit tests still pass after this change.

            Show
            timhunt Tim Hunt added a comment - Note, unit tests still pass after this change.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-40313

            • Remote repository: git://github.com/timhunt/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-40313 Remote repository: git://github.com/timhunt/moodle.git Remote branch MDL-40313 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/499 Warning: The MDL-40313 branch at git://github.com/timhunt/moodle.git has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/499/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            No objection from Ray yet, so I'm submitting this for integration.

            Show
            timhunt Tim Hunt added a comment - No objection from Ray yet, so I'm submitting this for integration.
            Hide
            raymor Ray Morris added a comment -

            > Ray, are you happy with the way I have messed around with your code?

            Sorry, I didn't see that question until now. I have reviewed the changes you made and see no issues.
            I will be looking to see if I need to do similar adjustments to MDL-40457 .

            Show
            raymor Ray Morris added a comment - > Ray, are you happy with the way I have messed around with your code? Sorry, I didn't see that question until now. I have reviewed the changes you made and see no issues. I will be looking to see if I need to do similar adjustments to MDL-40457 .
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your perseverance Ray. I've integrated this to master

            Show
            poltawski Dan Poltawski added a comment - Thanks for your perseverance Ray. I've integrated this to master
            Show
            poltawski Dan Poltawski added a comment - Note, unbuilt JS changes were discovered: http://integration.moodle.org/job/06.%20Run%20shifter%20for%20all%20modules%20(master)/1533/artifact/shifter_walk.txt So I had to commit the sniftered changes, hope they've been tested: http://git.moodle.org/gw?p=integration.git;a=commit;h=81f7090b0c510b8715e4d321c4b0684647e1489e
            Hide
            timhunt Tim Hunt added a comment -

            Sorry about that. If testing now finds problems, my preference now is to fix them, and keep moving forwards.

            Show
            timhunt Tim Hunt added a comment - Sorry about that. If testing now finds problems, my preference now is to fix them, and keep moving forwards.
            Hide
            markn Mark Nelson added a comment -

            Hi Ray,

            Thanks for working on this!

            Few points -

            1. I found "Ensure at least one question is deleted (hidden)" vague. Would have been nice to know the question had to be in use in order for this to work.
            2. When I went to create a quiz and add a question I received the error "Fatal error: Call to a member function display_options() on a non-object in /Users/mark/htdocs/mstorage/im/moodle/mod/quiz/editlib.php on line 1239".
            3. The searchbytags plugin causes the error "Fatal error: Class 'question_bank_search_condition' not found in /Users/mark/htdocs/mstorage/im/moodle/local/searchbytags/lib.php on line 26" which I had to fix by changing "extends question_bank_search_condition" to "extends \core_question\bank\search\condition".

            The second issue is the reason I am failing this, just wanted to point out the other issues I experienced.

            Show
            markn Mark Nelson added a comment - Hi Ray, Thanks for working on this! Few points - I found "Ensure at least one question is deleted (hidden)" vague. Would have been nice to know the question had to be in use in order for this to work. When I went to create a quiz and add a question I received the error "Fatal error: Call to a member function display_options() on a non-object in /Users/mark/htdocs/mstorage/im/moodle/mod/quiz/editlib.php on line 1239". The searchbytags plugin causes the error "Fatal error: Class 'question_bank_search_condition' not found in /Users/mark/htdocs/mstorage/im/moodle/local/searchbytags/lib.php on line 26" which I had to fix by changing "extends question_bank_search_condition" to "extends \core_question\bank\search\condition". The second issue is the reason I am failing this, just wanted to point out the other issues I experienced.
            Hide
            timhunt Tim Hunt added a comment -

            2. is also caused by a problem in the searchbytags add-on used in testing. You need to change local_searchbytags_get_question_bank_search_conditions to

            return array(new local_searchbytags_question_bank_search_condition());

            (This was another API change during development. Sorry.)

            Show
            timhunt Tim Hunt added a comment - 2. is also caused by a problem in the searchbytags add-on used in testing. You need to change local_searchbytags_get_question_bank_search_conditions to return array(new local_searchbytags_question_bank_search_condition()); (This was another API change during development. Sorry.)
            Hide
            timhunt Tim Hunt added a comment -

            In other words, I think this can be made a pass.

            Show
            timhunt Tim Hunt added a comment - In other words, I think this can be made a pass.
            Hide
            poltawski Dan Poltawski added a comment -

            Sending back to testing - Mark, can you confirm?

            Show
            poltawski Dan Poltawski added a comment - Sending back to testing - Mark, can you confirm?
            Hide
            markn Mark Nelson added a comment -

            Yep, works as expected now Dan.

            Just a note for Tim, there exists another issue in the searchbytags plugin.

            Coding error detected, it must be fixed by a programmer: moodle_database::get_in_or_equal() does not accept empty arrays
             
            More information about this error
            Debug info:
            Error code: codingerror
            Stack trace:
             
            line 680 of /lib/dml/moodle_database.php: coding_exception thrown
            line 67 of /local/searchbytags/lib.php: call to moodle_database->get_in_or_equal()
            line 36 of /local/searchbytags/lib.php: call to local_searchbytags_question_bank_search_condition->init()
            line 23 of /local/searchbytags/lib.php: call to local_searchbytags_question_bank_search_condition->__construct()
            line 953 of /question/editlib.php: call to local_searchbytags_get_question_bank_search_conditions()
            line 942 of /question/editlib.php: call to question_bank_view->init_search_conditions()
            line 39 of /question/edit.php: call to question_bank_view->__construct(
            

            Show
            markn Mark Nelson added a comment - Yep, works as expected now Dan. Just a note for Tim, there exists another issue in the searchbytags plugin. Coding error detected, it must be fixed by a programmer: moodle_database::get_in_or_equal() does not accept empty arrays   More information about this error Debug info: Error code: codingerror Stack trace:   line 680 of /lib/dml/moodle_database.php: coding_exception thrown line 67 of /local/searchbytags/lib.php: call to moodle_database->get_in_or_equal() line 36 of /local/searchbytags/lib.php: call to local_searchbytags_question_bank_search_condition->init() line 23 of /local/searchbytags/lib.php: call to local_searchbytags_question_bank_search_condition->__construct() line 953 of /question/editlib.php: call to local_searchbytags_get_question_bank_search_conditions() line 942 of /question/editlib.php: call to question_bank_view->init_search_conditions() line 39 of /question/edit.php: call to question_bank_view->__construct(
            Hide
            raymor Ray Morris added a comment -

            Sorry I forgot to update the attachment, the demo plugin.
            I'll try to get an updated plugin uploaded some time today.

            PS - there also exists a simple plugin to search the text of questions and optionally the text of answers, should anyone need it. It's handy for finding "all of the above" answers that shouldn't be shuffled, etc.

            Show
            raymor Ray Morris added a comment - Sorry I forgot to update the attachment, the demo plugin. I'll try to get an updated plugin uploaded some time today. PS - there also exists a simple plugin to search the text of questions and optionally the text of answers, should anyone need it. It's handy for finding "all of the above" answers that shouldn't be shuffled, etc.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Well done is better than well said.

            Closing, big thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Well done is better than well said. Closing, big thanks!
            Hide
            csaba.gloner Csaba Gloner added a comment -

            We couldn't install it on 2.7.
            Have anyone tried it?

            Show
            csaba.gloner Csaba Gloner added a comment - We couldn't install it on 2.7. Have anyone tried it?
            Hide
            raymor Ray Morris added a comment -

            Bela CSABI I'm sending you an email. In order for me to help you, it would be useful to know exactly what problem you're having. Something like filling in the blanks in the following sentence:

            When download ____, then I click on _________ , I get an error message saying ________ .

            It might also be helpful to enable debugging at Site Administration > Development > Debugging.

            To directly answer the question you actually asked, yes, other people have tried it and found it works for them. Before anything is integrated with Moodle, it's independently tested by at least two - three people.

            Show
            raymor Ray Morris added a comment - Bela CSABI I'm sending you an email. In order for me to help you, it would be useful to know exactly what problem you're having. Something like filling in the blanks in the following sentence: When download ____, then I click on _________ , I get an error message saying ________ . It might also be helpful to enable debugging at Site Administration > Development > Debugging. To directly answer the question you actually asked, yes, other people have tried it and found it works for them. Before anything is integrated with Moodle, it's independently tested by at least two - three people.
            Hide
            csaba.gloner Csaba Gloner added a comment - - edited

            Hi Ray,

            thanks for your quick answer.
            After copied the plugin to the local folder I couldn't start Moodle, I saw after the plugin check screen.

            Fatal error: Class 'question_bank_search_condition' not found in C:\moodle\server\moodle\local\searchbytags\lib.php on line 26

            My colleague also tried it and saw "fatal error was thrown trying to reference a particular function "

            Best regards
            Csaba

            Show
            csaba.gloner Csaba Gloner added a comment - - edited Hi Ray, thanks for your quick answer. After copied the plugin to the local folder I couldn't start Moodle, I saw after the plugin check screen. Fatal error: Class 'question_bank_search_condition' not found in C:\moodle\server\moodle\local\searchbytags\lib.php on line 26 My colleague also tried it and saw "fatal error was thrown trying to reference a particular function " Best regards Csaba
            Hide
            raymor Ray Morris added a comment -

            Updated plugin for testing, also includes Moodle 2.8 MDL-40457 "Allow plugins to add columns to question bank view".

            Show
            raymor Ray Morris added a comment - Updated plugin for testing, also includes Moodle 2.8 MDL-40457 "Allow plugins to add columns to question bank view".
            Hide
            raymor Ray Morris added a comment -

            You may have better luck with the newer version of the plugin I just attached. If not, please reply to my email.

            Show
            raymor Ray Morris added a comment - You may have better luck with the newer version of the plugin I just attached. If not, please reply to my email.

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14