Moodle
  1. Moodle
  2. MDL-27829

Pagetypes: Find a fix for core component and plugin overlaps before 2.1 release

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Blocks
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      To test you need to add a block to a variety of pages then edit the block's settings and check the "display on page types" drop down. The set of options that should be available are listed below. The ordering of the options isn't important.

      on a course main page:
      any page
      any course page
      any type of course page

      on a quiz:
      any quiz module page
      edit quiz page

      on a database activity:
      no drop down as this is handle automatically.

      on a course report like the course activity report (Courses > course name > Reports > activity report):
      any page
      any course report
      just the current course report ie "Activity course report"

      on an admin setting page like Advanced features (Site administration > Advanced features):
      The current admin setting page
      Any admin setting page

      Show
      To test you need to add a block to a variety of pages then edit the block's settings and check the "display on page types" drop down. The set of options that should be available are listed below. The ordering of the options isn't important. on a course main page: any page any course page any type of course page on a quiz: any quiz module page edit quiz page on a database activity: no drop down as this is handle automatically. on a course report like the course activity report (Courses > course name > Reports > activity report): any page any course report just the current course report ie "Activity course report" on an admin setting page like Advanced features (Site administration > Advanced features): The current admin setting page Any admin setting page
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-27829_block_pagetype_master
    • Rank (Obsolete):
      17515

      Description

      Hi Andrew,

      As discussed just before MDL-26105 has been integrated with master now but before the release of 2.1 we need to find a solution for overlap problems arising from a crossover between core components and plugins.
      To test the problem add a block to a course report page - inspect the block_instance table, edit the block and look at the setting options (all course related) and after saving watch what happens to the block_instance table.
      While discussing it we wondered about the idea of allowing overriding callbacks built by looking at the raw pagetype.

      Yell out if you have any questions.

      Cheers
      Sam

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          All yours Andrew

          Show
          Sam Hemelryk added a comment - All yours Andrew
          Hide
          Andrew Davis added a comment -

          I think I have a workable solution.

          https://github.com/andyjdavis/moodle/commit/fccde4fb076fb2d3848407d735f3787181173435

          feedback welcome. If you have suggestions on how to better implement this let me know

          Show
          Andrew Davis added a comment - I think I have a workable solution. https://github.com/andyjdavis/moodle/commit/fccde4fb076fb2d3848407d735f3787181173435 feedback welcome. If you have suggestions on how to better implement this let me know
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho Andrew, some quick comments:

          1) I think that search for functions should happen only for plugin pages.
          2) And plugin_pagetypelist() does basically that. Surely you only need to add fallback to parent dir there.
          3) IMO the problem is that the "if (array_key_exists($component, $plugins) " condition is completely wrong (only works for 1st level plugins, like mod/blocks, but not for the rest) and that isn't ever executed (for other plugin types. Also "$component" is messed there and does not point to proper "plugintype_pluginame_" style.

          Hope it helps ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho Andrew, some quick comments: 1) I think that search for functions should happen only for plugin pages. 2) And plugin_pagetypelist() does basically that. Surely you only need to add fallback to parent dir there. 3) IMO the problem is that the "if (array_key_exists($component, $plugins) " condition is completely wrong (only works for 1st level plugins, like mod/blocks, but not for the rest) and that isn't ever executed (for other plugin types. Also "$component" is messed there and does not point to proper "plugintype_pluginame_" style. Hope it helps ciao
          Hide
          Andrew Davis added a comment -
          Show
          Andrew Davis added a comment - moved strings out of the core string files. https://github.com/andyjdavis/moodle/commit/20fc347172c3ff31fd5309b284be81f39eda6f4c
          Hide
          Andrew Davis added a comment -

          Another issue. generate_page_type_patterns() isn't generating valid function names under all circumstances. If a callback for the outline course report were written it should be called coursereport_outline_pagetypelist(). Not coursereportoutline_pagetypelist().

          Show
          Andrew Davis added a comment - Another issue. generate_page_type_patterns() isn't generating valid function names under all circumstances. If a callback for the outline course report were written it should be called coursereport_outline_pagetypelist(). Not coursereportoutline_pagetypelist().
          Hide
          Tim Hunt added a comment -

          Some code-review on https://github.com/andyjdavis/moodle/commit/fccde4fb076fb2d3848407d735f3787181173435

          1. coursereport_pagetypelist does not follow the coding guidelines. Should be _page_type_list
          2. First array in that function is not properly laid out: http://docs.moodle.org/dev/Coding_style#Wrapping_Arrays or http://docs.moodle.org/dev/Coding_style#Associative_arrays
          3. if (count($bits >= 3)) { is wrong
          4. for($i should be for ($i
          5. implode($componentarray) - I had to look this up to confirm it was valid. I would always spell this out as implode('', $componentarray)
          6. Anyway, I am not sure that $component = implode($componentarray) works in any case other than coursereport. What about report -> admin/report; qtype -> question/type, ...
          Show
          Tim Hunt added a comment - Some code-review on https://github.com/andyjdavis/moodle/commit/fccde4fb076fb2d3848407d735f3787181173435 coursereport_pagetypelist does not follow the coding guidelines. Should be _page_type_list First array in that function is not properly laid out: http://docs.moodle.org/dev/Coding_style#Wrapping_Arrays or http://docs.moodle.org/dev/Coding_style#Associative_arrays if (count($bits >= 3)) { is wrong for($i should be for ($i implode($componentarray) - I had to look this up to confirm it was valid. I would always spell this out as implode('', $componentarray) Anyway, I am not sure that $component = implode($componentarray) works in any case other than coursereport. What about report -> admin/report; qtype -> question/type, ...
          Hide
          Tim Hunt added a comment -

          Now looking at https://github.com/andyjdavis/moodle/commit/20fc347172c3ff31fd5309b284be81f39eda6f4c

          1. I am not sure that 'Any log course report' etc. make sense. There is only one log report.
          2. It is a bit sucky to change the plugin API to require all course reports to add this string, for such a minor feature as this. Is it really necessary?

          As per dev chat:

          (10:57:08 AM) Eloy: andrew have you read my comment? it seems that plugin_pagetypelist() already does a big part of what you're trying to achieve. The only problem is that the:

          "if (array_key_exists($component, $plugins)"

          condition only works for 1st level plugins! (mod, blocks...) but not for the rest!
          (10:59:15 AM) Eloy: and of course "$component" is also wrong. Must be, always, plugintype_pluginame
          (10:59:22 AM) Eloy: cya
          (10:59:29 AM) timhunt: Yes. We need a reliable way to get the frankenstyle name of the plugin responsible for this page from $PAGE. I think we need a specific $PAGE->get_owning_component() method.
          (10:59:39 AM) timhunt: This logic should not be mixed up in generate_page_type_patterns
          (10:59:43 AM) Eloy: yup

          Show
          Tim Hunt added a comment - Now looking at https://github.com/andyjdavis/moodle/commit/20fc347172c3ff31fd5309b284be81f39eda6f4c I am not sure that 'Any log course report' etc. make sense. There is only one log report. It is a bit sucky to change the plugin API to require all course reports to add this string, for such a minor feature as this. Is it really necessary? As per dev chat: (10:57:08 AM) Eloy: andrew have you read my comment? it seems that plugin_pagetypelist() already does a big part of what you're trying to achieve. The only problem is that the: "if (array_key_exists($component, $plugins)" condition only works for 1st level plugins! (mod, blocks...) but not for the rest! (10:59:15 AM) Eloy: and of course "$component" is also wrong. Must be, always, plugintype_pluginame (10:59:22 AM) Eloy: cya (10:59:29 AM) timhunt: Yes. We need a reliable way to get the frankenstyle name of the plugin responsible for this page from $PAGE. I think we need a specific $PAGE->get_owning_component() method. (10:59:39 AM) timhunt: This logic should not be mixed up in generate_page_type_patterns (10:59:43 AM) Eloy: yup
          Hide
          Andrew Davis added a comment - - edited

          Ive started fixing this stuff. Branch is MDL-27829_block_page_type_fixes Just recording this here so I know what branch to pull when I get home.

          Show
          Andrew Davis added a comment - - edited Ive started fixing this stuff. Branch is MDL-27829 _block_page_type_fixes Just recording this here so I know what branch to pull when I get home.
          Hide
          Andrew Davis added a comment - - edited

          Ok. Ive heavily reworked this. Far fewer lib files will be loaded although the page type will be pulled apart twice. Probably more work to do there.

          Note that this isnt finished code. Theres still commented out code lying around in this plus I havent tested this particularly thoroughly. Just after some general feedback from those in the know.

          Most of the action is in generate_page_type_patterns().

          Im currently intending on creating functions for each course report ie outline_page_type_list() for coursereport_outline. coursereport_page_type_list() will disappear. That means more functions but the functions will be really simple pieces of code. I figured I was trying to be a bit fancy and possibly creating problems for the future trying to do this dynamically.

          Show
          Andrew Davis added a comment - - edited Ok. Ive heavily reworked this. Far fewer lib files will be loaded although the page type will be pulled apart twice. Probably more work to do there. Note that this isnt finished code. Theres still commented out code lying around in this plus I havent tested this particularly thoroughly. Just after some general feedback from those in the know. Most of the action is in generate_page_type_patterns(). Im currently intending on creating functions for each course report ie outline_page_type_list() for coursereport_outline. coursereport_page_type_list() will disappear. That means more functions but the functions will be really simple pieces of code. I figured I was trying to be a bit fancy and possibly creating problems for the future trying to do this dynamically.
          Hide
          Andrew Davis added a comment - - edited

          Ive created the various course report callbacks. Ive also been able to remove the need to iterate through the page type more than once. I think this is more or less ready. Peer review requested.

          Show
          Andrew Davis added a comment - - edited Ive created the various course report callbacks. Ive also been able to remove the need to iterate through the page type more than once. I think this is more or less ready. Peer review requested.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          I've just been looking over this now.
          In regards to the patch I noticed only a couple of minor things that don't change functionality but really just help the readability of the generate_page_type_patterns function.
          *lib/blocklib.php

            • ln 1593: Is there any need to the array_slice, surely $pluginname = $bits[$i] is fine?
            • ln 1582: On this line you check whether the result of the function call is empty but on lines 1607 and 1617 you don't.
            • ln 1613: $patterns will always be empty
            • ln 1626: Is there any need for this last !empty check of $patterns, we know that default_page_type_list will always return at least the $patterns array being built within that if.

          Other than that I think the patch is looking good.

          There is one thing worth nothing however and that is that I REALLY dislike the function names being used by this function + callbacks.
          They were function names I think I used when I first wrote the bit of code to make use of core component functions and I really regret the way I've named them.
          It doesn't really have any bearing on this issue, I will create an issue shortly when I have time to gather my thoughts about it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, I've just been looking over this now. In regards to the patch I noticed only a couple of minor things that don't change functionality but really just help the readability of the generate_page_type_patterns function. *lib/blocklib.php ln 1593: Is there any need to the array_slice, surely $pluginname = $bits [$i] is fine? ln 1582: On this line you check whether the result of the function call is empty but on lines 1607 and 1617 you don't. ln 1613: $patterns will always be empty ln 1626: Is there any need for this last !empty check of $patterns, we know that default_page_type_list will always return at least the $patterns array being built within that if. Other than that I think the patch is looking good. There is one thing worth nothing however and that is that I REALLY dislike the function names being used by this function + callbacks. They were function names I think I used when I first wrote the bit of code to make use of core component functions and I really regret the way I've named them. It doesn't really have any bearing on this issue, I will create an issue shortly when I have time to gather my thoughts about it. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Just found a warning when adding a block to the course report listing screen:

          Notice: Undefined offset: 0 in /var/git/peer-review/master/lib/blocklib.php on line 1594
          Call Stack

          1. Time Memory Function Location
            1 0.0003 662080
            Unknown macro: {main}

            ( ) ../report.php:0
            2 0.0795 24029992 bootstrap_renderer->header( ) ../report.php:20
            3 0.0795 24030384 bootstrap_renderer->__call( ) ../setuplib.php:0
            4 0.0812 24116744 call_user_func_array ( ) ../setuplib.php:1245
            5 0.0812 24117112 core_renderer->header( ) ../setuplib.php:0
            6 0.0812 24117192 moodle_page->set_state( ) ../outputrenderers.php:607
            7 0.0812 24117192 moodle_page->starting_output( ) ../pagelib.php:718
            8 0.0817 24139272 block_manager->process_url_actions( ) ../pagelib.php:1206
            9 0.0818 24139272 block_manager->process_url_edit( ) ../blocklib.php:1047
            10 0.1201 34184536 block_edit_form->__construct( ) ../blocklib.php:1190
            11 0.1201 34184536 moodleform->moodleform( ) ../edit_form.php:59
            12 0.1208 34209736 block_edit_form->definition( ) ../formslib.php:152
            13 0.1441 37525720 generate_page_type_patterns( ) ../edit_form.php:132

          Show
          Sam Hemelryk added a comment - Just found a warning when adding a block to the course report listing screen: Notice: Undefined offset: 0 in /var/git/peer-review/master/lib/blocklib.php on line 1594 Call Stack Time Memory Function Location 1 0.0003 662080 Unknown macro: {main} ( ) ../report.php:0 2 0.0795 24029992 bootstrap_renderer->header( ) ../report.php:20 3 0.0795 24030384 bootstrap_renderer->__call( ) ../setuplib.php:0 4 0.0812 24116744 call_user_func_array ( ) ../setuplib.php:1245 5 0.0812 24117112 core_renderer->header( ) ../setuplib.php:0 6 0.0812 24117192 moodle_page->set_state( ) ../outputrenderers.php:607 7 0.0812 24117192 moodle_page->starting_output( ) ../pagelib.php:718 8 0.0817 24139272 block_manager->process_url_actions( ) ../pagelib.php:1206 9 0.0818 24139272 block_manager->process_url_edit( ) ../blocklib.php:1047 10 0.1201 34184536 block_edit_form->__construct( ) ../blocklib.php:1190 11 0.1201 34184536 moodleform->moodleform( ) ../edit_form.php:59 12 0.1208 34209736 block_edit_form->definition( ) ../formslib.php:152 13 0.1441 37525720 generate_page_type_patterns( ) ../edit_form.php:132
          Hide
          Andrew Davis added a comment -

          Ok. I think thats all done.

          Show
          Andrew Davis added a comment - Ok. I think thats all done.
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          I've had a look at this and it get's my +1.
          I've made you the integrator as you've done the previous reviews and helped out with it etc...
          Let me know if you don't have time and I will give it a final once over before putting it in myself.

          Cheerio
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, I've had a look at this and it get's my +1. I've made you the integrator as you've done the previous reviews and helped out with it etc... Let me know if you don't have time and I will give it a final once over before putting it in myself. Cheerio Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam, I'm moving this back to you as far as I assume it's fresher in your mind. So I will, for sure, be able to do other 5x integrations with the same effort.

          If at the end of the day I've time to take a look to this I will (although I'm 99% sure I won't). Thanks for all the effort Andrew!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam, I'm moving this back to you as far as I assume it's fresher in your mind. So I will, for sure, be able to do other 5x integrations with the same effort. If at the end of the day I've time to take a look to this I will (although I'm 99% sure I won't). Thanks for all the effort Andrew! Ciao
          Hide
          Sam Hemelryk added a comment -

          This has been integrated now thanks Andrew.

          Show
          Sam Hemelryk added a comment - This has been integrated now thanks Andrew.
          Hide
          Helen Foster added a comment -

          Tried logging in as a teacher, adding a block to a course page and then configuring the block. I obtained the following options:

          • Any type of course main page
          • Any page
          • Any course page
          • Any activity module page

          The option 'Any activity module page' wasn't available previously. It doesn't seem a sensible option to me. If selected, the block doesn't appear on the course page but instead appears on every activity module page i.e. a lot of work to remove if added by accident! Thus I would recommend that this new option is removed.

          Show
          Helen Foster added a comment - Tried logging in as a teacher, adding a block to a course page and then configuring the block. I obtained the following options: Any type of course main page Any page Any course page Any activity module page The option 'Any activity module page' wasn't available previously. It doesn't seem a sensible option to me. If selected, the block doesn't appear on the course page but instead appears on every activity module page i.e. a lot of work to remove if added by accident! Thus I would recommend that this new option is removed.
          Hide
          Helen Foster added a comment -

          Tried adding a block to a quiz activity and noticed the setting 'Display on page types' is missing. It is available when adding a block to an assignment activity and is also available when adding a block to a quiz activity in 2.0.3+.

          Show
          Helen Foster added a comment - Tried adding a block to a quiz activity and noticed the setting 'Display on page types' is missing. It is available when adding a block to an assignment activity and is also available when adding a block to a quiz activity in 2.0.3+.
          Hide
          Helen Foster added a comment -

          Sorry I'm failing this issue as, after following the testing instructions 'check that the "display on page types" drop down containing a sensible set of options', I have found that the options are not sensible or in the case of quiz activities, missing entirely.

          Show
          Helen Foster added a comment - Sorry I'm failing this issue as, after following the testing instructions 'check that the "display on page types" drop down containing a sensible set of options', I have found that the options are not sensible or in the case of quiz activities, missing entirely.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Many thanks, Helen!

          A) Well, there are 2 (alternative) ways here:

          1) The test instructions were way too general for this issue. So then, they must be better specified, so this can be retested and passed.
          2) The test instructions are correct and the problems reported by Helen are related to this, so we must revert this or fix in in the next 16 hours.

          B) And there is also one conclusion: The test: "check that the "display on" dropdown won't pass, no matter which issue is the responsible (I bet it was MDL-26105 the one introducing the problem). But the fact is that the problem exists. One again, there are two alternatives:

          1) Move the problems to recently created MDL-27829 and ignore.
          2) Fix the wrong behaviors in the next 16h.

          So, guys, you've to decide about both A) and B) ASAP!

          Ciao

          PS: Moodle 2.2 will stop accepting improvements and new features at least 1 month before release. All these issues open hours before release cannot happen again IMO (just a reflexion).

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Many thanks, Helen! A) Well, there are 2 (alternative) ways here: 1) The test instructions were way too general for this issue. So then, they must be better specified, so this can be retested and passed. 2) The test instructions are correct and the problems reported by Helen are related to this, so we must revert this or fix in in the next 16 hours. B) And there is also one conclusion: The test: "check that the "display on" dropdown won't pass, no matter which issue is the responsible (I bet it was MDL-26105 the one introducing the problem). But the fact is that the problem exists. One again, there are two alternatives: 1) Move the problems to recently created MDL-27829 and ignore. 2) Fix the wrong behaviors in the next 16h. So, guys, you've to decide about both A) and B) ASAP! Ciao PS: Moodle 2.2 will stop accepting improvements and new features at least 1 month before release. All these issues open hours before release cannot happen again IMO (just a reflexion).
          Hide
          Andrew Davis added a comment - - edited

          Eloy, you are correct. This world of hurt was introduced by MDL-26105. I may print out a big banner that says "Thou shalt not rush through big changes just before release". It should probably go over my desk

          I was going to do further bug fixing in MDL-27893 but instead I think I will close that issue and do it against this one.

          Show
          Andrew Davis added a comment - - edited Eloy, you are correct. This world of hurt was introduced by MDL-26105 . I may print out a big banner that says "Thou shalt not rush through big changes just before release". It should probably go over my desk I was going to do further bug fixing in MDL-27893 but instead I think I will close that issue and do it against this one.
          Hide
          Andrew Davis added a comment -

          Comment by me in MDL-27893:

          Here are the various page types you can in the various areas. Ive added notes describing those that were present in 2.0.3 which don't exist in 2.1

          front page:

          site-index
          site-index-*
          *
          

          (perfect match)

          course:

          course-*       any course page
          course-view-*  any type of course main page
          mod-*          any activity module page
          *              any page
          

          2 additional options were available in 2.0.3

          course-view-weeks   any course main page in weeks format
          course-view-weeks-* ?
          

          activity:
          no options given. Need to check what page type is being used.
          additional options were available in 2.0.3

          mod-assignment-view
          mod-*-view          any main activity module page
          mod-assignment-view-*
          mod-assignment-*
          mod-*               any activity module page
          *                   any page
          

          course report (outline report used as example):

          course-report-outline-*  course activity report
          course-report-*          any course report
          *                        any page
          

          Additional options were available in 2.0.3

          course-report-outline-index
          course-report-outline-index-*
          course-*
          

          admin page:

          front page only
          front page and any pages added to the front page
          entire site
          

          This is clearly wrong. Here are the 2.0.3 options

          admin-setting-optionalsubsystems
          admin-setting-optionalsubsystems-*
          admin-setting-*
          admin-*
          *
          

          my moodle
          no options given. Need to check what page type is being used.
          additional options were available in 2.0.3

          my-index
          my-index-*
          my-*
          *
          
          Show
          Andrew Davis added a comment - Comment by me in MDL-27893 : Here are the various page types you can in the various areas. Ive added notes describing those that were present in 2.0.3 which don't exist in 2.1 front page: site-index site-index-* * (perfect match) course: course-* any course page course-view-* any type of course main page mod-* any activity module page * any page 2 additional options were available in 2.0.3 course-view-weeks any course main page in weeks format course-view-weeks-* ? activity: no options given. Need to check what page type is being used. additional options were available in 2.0.3 mod-assignment-view mod-*-view any main activity module page mod-assignment-view-* mod-assignment-* mod-* any activity module page * any page course report (outline report used as example): course-report-outline-* course activity report course-report-* any course report * any page Additional options were available in 2.0.3 course-report-outline-index course-report-outline-index-* course-* admin page: front page only front page and any pages added to the front page entire site This is clearly wrong. Here are the 2.0.3 options admin-setting-optionalsubsystems admin-setting-optionalsubsystems-* admin-setting-* admin-* * my moodle no options given. Need to check what page type is being used. additional options were available in 2.0.3 my-index my-index-* my-* *
          Hide
          Andrew Davis added a comment -

          "The option 'Any activity module page' wasn't available previously. It doesn't seem a sensible option to me. If selected, the block doesn't appear on the course page but instead appears on every activity module page i.e. a lot of work to remove if added by accident! Thus I would recommend that this new option is removed."

          A block added to "any activity module page" doesnt appear on the course page but does appear on all module pages. Its still a single block so if you delete it from any of those activities its deleted from all of them. All the same, it is a little weird so unless theres real demand for it (there isnt afaik) its gone.

          Show
          Andrew Davis added a comment - "The option 'Any activity module page' wasn't available previously. It doesn't seem a sensible option to me. If selected, the block doesn't appear on the course page but instead appears on every activity module page i.e. a lot of work to remove if added by accident! Thus I would recommend that this new option is removed." A block added to "any activity module page" doesnt appear on the course page but does appear on all module pages. Its still a single block so if you delete it from any of those activities its deleted from all of them. All the same, it is a little weird so unless theres real demand for it (there isnt afaik) its gone.
          Hide
          Andrew Davis added a comment -

          I spoke to Dongsheng about this issue. He was the original author of much of the code and I wasn't party to the original discussions about what was to be done. Apparently there was a conscious decision to discard some of the options as some of them were identical in practice.

          If a block has been configured to use an option that has been removed it will continue to work. You just won't be able to add new blocks with the same page types setting. Editing a block set to a now removed page type could be a bit weird.

          Show
          Andrew Davis added a comment - I spoke to Dongsheng about this issue. He was the original author of much of the code and I wasn't party to the original discussions about what was to be done. Apparently there was a conscious decision to discard some of the options as some of them were identical in practice. If a block has been configured to use an option that has been removed it will continue to work. You just won't be able to add new blocks with the same page types setting. Editing a block set to a now removed page type could be a bit weird.
          Hide
          Andrew Davis added a comment - - edited

          Adding a block to an activity, I used database for this example, causes the block to have its page type restriction set to mod-data-view. I suspect that the 2.0.3 set of options has been whittled down to a single option. As there is only a single option the drop down box is omitted from the block setting page.

          Similarly, blocks added to the my moodle page automatically get the page type of my-index rather than the set of mostly pointless options. I believe this is by design.

          Show
          Andrew Davis added a comment - - edited Adding a block to an activity, I used database for this example, causes the block to have its page type restriction set to mod-data-view. I suspect that the 2.0.3 set of options has been whittled down to a single option. As there is only a single option the drop down box is omitted from the block setting page. Similarly, blocks added to the my moodle page automatically get the page type of my-index rather than the set of mostly pointless options. I believe this is by design.
          Hide
          Andrew Davis added a comment - - edited

          SUMMARY: I believe are three things I have to do.

          1) the set of page types presented for a block on a admin settings page is incorrect.

          2) the testing instructions need to be improved.

          3) add a block then manually edit the database to set its page type to be one of the removed page types. check that nothing bad happens.

          Show
          Andrew Davis added a comment - - edited SUMMARY: I believe are three things I have to do. 1) the set of page types presented for a block on a admin settings page is incorrect. 2) the testing instructions need to be improved. 3) add a block then manually edit the database to set its page type to be one of the removed page types. check that nothing bad happens.
          Hide
          Andrew Davis added a comment - - edited

          #1 there is a new commit that fixes admin setting pages. https://github.com/andyjdavis/moodle/commit/800688bd1ef460c6447779d77780c16fc90348a7

          #2 testing instructions updated.

          #3 There was some code to deal with the removal of selectable page types however it was broken. Somewhat fixed now https://github.com/andyjdavis/moodle/commit/096928c79e4ed6f0145de4099b06cbeb6dbc9b62

          There is a bit of an issue with dealing with blocks set to show on page types which we no longer allow the user to select. I added code to add the page type to the list of options but then you wind up with mod-data-view being added as it doesnt match mod-data-*

          I then added some intelligence so that it realized that mod-data-view matches mod-data-* but then it stops adding course-view-weeks because it finds course-view-*.

          Right now, it will add a new option if the block specifies a now non-selectable page type and it can find an appropriate string. Otherwise your block continues to work fine but when you open its settings the drop down box will just default to the first option.

          Show
          Andrew Davis added a comment - - edited #1 there is a new commit that fixes admin setting pages. https://github.com/andyjdavis/moodle/commit/800688bd1ef460c6447779d77780c16fc90348a7 #2 testing instructions updated. #3 There was some code to deal with the removal of selectable page types however it was broken. Somewhat fixed now https://github.com/andyjdavis/moodle/commit/096928c79e4ed6f0145de4099b06cbeb6dbc9b62 There is a bit of an issue with dealing with blocks set to show on page types which we no longer allow the user to select. I added code to add the page type to the list of options but then you wind up with mod-data-view being added as it doesnt match mod-data-* I then added some intelligence so that it realized that mod-data-view matches mod-data-* but then it stops adding course-view-weeks because it finds course-view-*. Right now, it will add a new option if the block specifies a now non-selectable page type and it can find an appropriate string. Otherwise your block continues to work fine but when you open its settings the drop down box will just default to the first option.
          Hide
          Andrew Davis added a comment -

          Eloy, can you please have a look at the two commits in my last comment?

          Helen, if Eloy's happy with them, once they're integrated this can be retested.

          Show
          Andrew Davis added a comment - Eloy, can you please have a look at the two commits in my last comment? Helen, if Eloy's happy with them, once they're integrated this can be retested.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, I guess you are talking about:

          • 800688bd1ef460c6447779d77780c16fc90348a7 (admin)
          • 096928c79e4ed6f0145de4099b06cbeb6dbc9b62 (the typo)
          • d097ad564df6facad0e6cf8937b91610eeb61d62 (the display warning)

          (the 3 last commits in the MDL-27829_block_pagetype_master branch)

          They look perfect, I'm going to re-integrate them to ignite a new testing round. Let's see how it behaves now.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, I guess you are talking about: 800688bd1ef460c6447779d77780c16fc90348a7 (admin) 096928c79e4ed6f0145de4099b06cbeb6dbc9b62 (the typo) d097ad564df6facad0e6cf8937b91610eeb61d62 (the display warning) (the 3 last commits in the MDL-27829 _block_pagetype_master branch) They look perfect, I'm going to re-integrate them to ignite a new testing round. Let's see how it behaves now. Thanks!
          Hide
          Helen Foster added a comment -

          Failing as two out of the four items listed in the testing instructions were not as described:

          1. on a course main page:
          any page
          any course page
          any type of course page - PASS

          2. on an activity within a course:
          you shouldn't get a drop down box at all as this is handled automatically - FAIL

          Adding a block to an assignment activity, the block's settings had a display on page types dropdown with the following options:

          Assignment module main page
          Any assignment module page
          Assignment module submissions page

          Whilst I'm all in favour of simplifying the UI where possible, I'm concerned that removing the display on page types dropdown completely is taking things too far. Certain modules e.g. assignment and quiz have a variety of different pages. In Moodle 2.0 the new blocks functionality enabled blocks to be added to certain activity module pages but not all. For example, quiz included the following options:

          mod-quiz-view
          mod-*-view
          mod-quiz-*

          This meant that a block could be added to mod/quiz/view.php but not mod/quiz/report.php for example.

          I would recommend that we think carefully before removing functionality that people may be depending upon.

          3. on a course report like the course activity report (Courses > course name > Reports > activity report):
          any page
          any course report
          just the current course report ie "Course activity report" it should actually tell you the name of the report youre looking at) - FAIL

          Adding a block to the activity completion report, the block's settings had a display on page types dropdown which included the option 'Progress course report' instead of 'Activity completion report'.

          Also it stated 'The previously specified page type is no longer selectable. Please choose the most appropriate page type below.' No page type was specified previously, as the block was newly created!

          I tried adding several different blocks to the activity completion report page, all with the same results.

          4. on an admin setting page like Advanced features (Site administration > Advanced features):
          The current admin setting page
          Any admin setting page - PASS

          Show
          Helen Foster added a comment - Failing as two out of the four items listed in the testing instructions were not as described: 1. on a course main page: any page any course page any type of course page - PASS 2. on an activity within a course: you shouldn't get a drop down box at all as this is handled automatically - FAIL Adding a block to an assignment activity, the block's settings had a display on page types dropdown with the following options: Assignment module main page Any assignment module page Assignment module submissions page Whilst I'm all in favour of simplifying the UI where possible, I'm concerned that removing the display on page types dropdown completely is taking things too far. Certain modules e.g. assignment and quiz have a variety of different pages. In Moodle 2.0 the new blocks functionality enabled blocks to be added to certain activity module pages but not all. For example, quiz included the following options: mod-quiz-view mod-*-view mod-quiz-* This meant that a block could be added to mod/quiz/view.php but not mod/quiz/report.php for example. I would recommend that we think carefully before removing functionality that people may be depending upon. 3. on a course report like the course activity report (Courses > course name > Reports > activity report): any page any course report just the current course report ie "Course activity report" it should actually tell you the name of the report youre looking at) - FAIL Adding a block to the activity completion report, the block's settings had a display on page types dropdown which included the option 'Progress course report' instead of 'Activity completion report'. Also it stated 'The previously specified page type is no longer selectable. Please choose the most appropriate page type below.' No page type was specified previously, as the block was newly created! I tried adding several different blocks to the activity completion report page, all with the same results. 4. on an admin setting page like Advanced features (Site administration > Advanced features): The current admin setting page Any admin setting page - PASS
          Hide
          Eloy Lafuente (stronk7) added a comment -

          About 2) Yes, really i think that the (previously existing) missing options in some activities like quiz, is going too far. It sounds to me that such feature was used also for deciding which pages in one attempt show a given block (note I'm not 100% sure about this, perhaps that is just a dream, lol). Also sounds reasonable to have access to all the subplugins available (quiz reports), like assignment types and so on.

          About 3) yup, needs fixing too. Also the newly introduced "The previously specified page type is no longer ..." string... why is it showing there? One new block should not require any conversion from 2.0.x options, so shouldn't be shown. That escaped for sure to my review, apologizes.

          So this really needs a certain degree of improvement. I think Helen has explained it perfectly in the previous comment, thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - About 2) Yes, really i think that the (previously existing) missing options in some activities like quiz, is going too far. It sounds to me that such feature was used also for deciding which pages in one attempt show a given block (note I'm not 100% sure about this, perhaps that is just a dream, lol). Also sounds reasonable to have access to all the subplugins available (quiz reports), like assignment types and so on. About 3) yup, needs fixing too. Also the newly introduced "The previously specified page type is no longer ..." string... why is it showing there? One new block should not require any conversion from 2.0.x options, so shouldn't be shown. That escaped for sure to my review, apologizes. So this really needs a certain degree of improvement. I think Helen has explained it perfectly in the previous comment, thanks! Ciao
          Hide
          Andrew Davis added a comment - - edited

          re #2, each module returns a set of page types. If theres only 1 page type no option is displayed to the user. If there is more than 1 the drop down is displayed. I guess I need to go through each activity module and figure out which had options that were removed and out of those which need to go back in.

          #3 Ive fixed up the strings. It was saying 'Progress course report' instead of 'Activity completion report' because that report is called the progress report internally but its called the activity completion report in the UI and I was confused. Eloy, there is a new commit in the branch.
          https://github.com/andyjdavis/moodle/commit/1fe0ecabf15f6543227be821fbe7c7d772755f43

          The "The previously specified page type is no longer selectable. Please choose the most appropriate page type below." message appears to be being displayed sometimes when it shouldn't be. I'll figure out why.

          Show
          Andrew Davis added a comment - - edited re #2, each module returns a set of page types. If theres only 1 page type no option is displayed to the user. If there is more than 1 the drop down is displayed. I guess I need to go through each activity module and figure out which had options that were removed and out of those which need to go back in. #3 Ive fixed up the strings. It was saying 'Progress course report' instead of 'Activity completion report' because that report is called the progress report internally but its called the activity completion report in the UI and I was confused. Eloy, there is a new commit in the branch. https://github.com/andyjdavis/moodle/commit/1fe0ecabf15f6543227be821fbe7c7d772755f43 The "The previously specified page type is no longer selectable. Please choose the most appropriate page type below." message appears to be being displayed sometimes when it shouldn't be. I'll figure out why.
          Hide
          Andrew Davis added a comment -

          2.0.3 assignment:
          mod-assignment-view
          mod-*-view
          mod-assignment-view-*
          mod-assignment-*
          mod-*
          *

          2.1 assignment:
          mod-assignment-*
          mod-assignment-view
          mod-assignment-submissions

          I think that looks fairly reasonable. I think...

          Show
          Andrew Davis added a comment - 2.0.3 assignment: mod-assignment-view mod-*-view mod-assignment-view-* mod-assignment-* mod-* * 2.1 assignment: mod-assignment-* mod-assignment-view mod-assignment-submissions I think that looks fairly reasonable. I think...
          Hide
          Andrew Davis added a comment - - edited

          2.0.3 quiz:
          mod-quiz-edit
          mod-quiz-edit-*
          mod-quiz-*
          mod-*
          *

          2.1 quiz:
          mod-quiz-*
          mod-quiz-edit (I just added this)

          Show
          Andrew Davis added a comment - - edited 2.0.3 quiz: mod-quiz-edit mod-quiz-edit-* mod-quiz-* mod-* * 2.1 quiz: mod-quiz-* mod-quiz-edit (I just added this)
          Hide
          Andrew Davis added a comment -

          2.0.3 glossary:
          mod-glossary-view
          mod-*-view
          mod-glossary-view-*
          mod-glossary-*
          mod-*
          *

          2.1 glossary:
          mod-glossary-*
          mod-glossary-view (I just added this)
          mod-glossary-edit (I just added this)

          Show
          Andrew Davis added a comment - 2.0.3 glossary: mod-glossary-view mod-*-view mod-glossary-view-* mod-glossary-* mod-* * 2.1 glossary: mod-glossary-* mod-glossary-view (I just added this) mod-glossary-edit (I just added this)
          Hide
          Andrew Davis added a comment - - edited

          2.0.3 lesson:
          mod-lesson-edit
          mod-lesson-edit-*
          mod-lesson-*
          mod-*
          *

          2.1 lesson:
          mod-lesson-*
          mod-lesson-view (just added)
          mod-lesson-edit (just added)
          mod-lesson-report (could add)
          mod-lesson-essay (could add)

          Because lesson supports sub pages an additional drop down box appears containing "any page matching the above" and "this specific page". I do not pretend to entirely understand what this does.

          Show
          Andrew Davis added a comment - - edited 2.0.3 lesson: mod-lesson-edit mod-lesson-edit-* mod-lesson-* mod-* * 2.1 lesson: mod-lesson-* mod-lesson-view (just added) mod-lesson-edit (just added) mod-lesson-report (could add) mod-lesson-essay (could add) Because lesson supports sub pages an additional drop down box appears containing "any page matching the above" and "this specific page". I do not pretend to entirely understand what this does.
          Hide
          Andrew Davis added a comment -

          Ive committed changes to add some more options (see the above posts). Also in that commit is changes to the course report page type list functions to prevent the incorrect display of the missing page type message.

          So Eloy, there are two new commits now.

          1) course report strings.
          https://github.com/andyjdavis/moodle/commit/1fe0ecabf15f6543227be821fbe7c7d772755f43

          2) added page types that I think we want.
          https://github.com/andyjdavis/moodle/commit/346a32a75b4fc1b0aa51355424d9d41a0c251fa2

          I'm going to continue comparing activities between 2.0.3 and 2.1 so a third commit may follow.

          Show
          Andrew Davis added a comment - Ive committed changes to add some more options (see the above posts). Also in that commit is changes to the course report page type list functions to prevent the incorrect display of the missing page type message. So Eloy, there are two new commits now. 1) course report strings. https://github.com/andyjdavis/moodle/commit/1fe0ecabf15f6543227be821fbe7c7d772755f43 2) added page types that I think we want. https://github.com/andyjdavis/moodle/commit/346a32a75b4fc1b0aa51355424d9d41a0c251fa2 I'm going to continue comparing activities between 2.0.3 and 2.1 so a third commit may follow.
          Hide
          Andrew Davis added a comment -

          2.0.3 forum:
          mod-forum-view
          mod-*-view
          mod-forum-view-*
          mod-forum-*
          mod-*
          *

          2.1 forum:
          mod-forum-*
          mod-forum-view
          mod-forum-discuss

          I think this looks ok.

          Show
          Andrew Davis added a comment - 2.0.3 forum: mod-forum-view mod-*-view mod-forum-view-* mod-forum-* mod-* * 2.1 forum: mod-forum-* mod-forum-view mod-forum-discuss I think this looks ok.
          Hide
          Andrew Davis added a comment -

          2.0.3 wiki:
          mod-wiki-view
          mod-*-view
          mod-wiki-view-*
          mod-wiki-*
          mod-*
          *

          2.1 wiki:
          mod-wiki-*
          mod-wiki-view
          mod-wiki-comments
          mod-wiki-history
          mod-wiki-map

          I think this looks ok.

          Show
          Andrew Davis added a comment - 2.0.3 wiki: mod-wiki-view mod-*-view mod-wiki-view-* mod-wiki-* mod-* * 2.1 wiki: mod-wiki-* mod-wiki-view mod-wiki-comments mod-wiki-history mod-wiki-map I think this looks ok.
          Hide
          Andrew Davis added a comment -

          Further improved testing instructions.

          Show
          Andrew Davis added a comment - Further improved testing instructions.
          Hide
          Andrew Davis added a comment - - edited

          Added a third commit since the last round of integration and testing.

          https://github.com/andyjdavis/moodle/commit/2f9bdadfd23321951a7fbf7a33ddc5af57c5ef74

          I thought all site admin pages have page types like admin-setting-* however ones exist like admin-registration-index so if you want a block to display on all of them it needs to be admin-*

          Show
          Andrew Davis added a comment - - edited Added a third commit since the last round of integration and testing. https://github.com/andyjdavis/moodle/commit/2f9bdadfd23321951a7fbf7a33ddc5af57c5ef74 I thought all site admin pages have page types like admin-setting-* however ones exist like admin-registration-index so if you want a block to display on all of them it needs to be admin-*
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Re-re integrated, thanks Andrew!

          Show
          Eloy Lafuente (stronk7) added a comment - Re-re integrated, thanks Andrew!
          Hide
          Andrew Davis added a comment -

          crosses fingers

          Show
          Andrew Davis added a comment - crosses fingers
          Hide
          Eloy Lafuente (stronk7) added a comment -

          After one more iteration we have found some problems that will be alleviated / fixed by these commits:

          1) e5a863e55c3dcb8067cb5ce4bcdc8689bbda45b3 - fixed one course report string
          2) ddaa9147895528f9baa5d6ec568f49c4db7a7220 - on creation of blocks, ensure that the pagetypepattern inserted is a valid one (that makes the message in the form to disappear for just created pages). Note this is only being applied to /mod pages, so other pages like course-report will continue showing the message. MDL-28150 (and MDL-28149) have been created about to decide if the /mod change should be applied ASAP to all the rest of pages. (this patch was peer-reviewed by Tim).
          3) 3e16a2ca66ff2e05125e22b78fe990d9ee479b44 - clean one dupe comment in code
          4) 715a5bb732b6d8c5875464ffd3b80c4ece5c5b0a - some more lang strings

          And that's all, about to integrate them in order to perform one more testing...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - After one more iteration we have found some problems that will be alleviated / fixed by these commits: 1) e5a863e55c3dcb8067cb5ce4bcdc8689bbda45b3 - fixed one course report string 2) ddaa9147895528f9baa5d6ec568f49c4db7a7220 - on creation of blocks, ensure that the pagetypepattern inserted is a valid one (that makes the message in the form to disappear for just created pages). Note this is only being applied to /mod pages, so other pages like course-report will continue showing the message. MDL-28150 (and MDL-28149 ) have been created about to decide if the /mod change should be applied ASAP to all the rest of pages. (this patch was peer-reviewed by Tim). 3) 3e16a2ca66ff2e05125e22b78fe990d9ee479b44 - clean one dupe comment in code 4) 715a5bb732b6d8c5875464ffd3b80c4ece5c5b0a - some more lang strings And that's all, about to integrate them in order to perform one more testing... Ciao
          Hide
          Helen Foster added a comment -

          Yes! Finally this test passes Many thanks to Andrew and Eloy.

          Small problem of 'The previously specified page type is no longer selectable...' being displayed on a non mod page is mentioned in MDL-28150.

          Show
          Helen Foster added a comment - Yes! Finally this test passes Many thanks to Andrew and Eloy. Small problem of 'The previously specified page type is no longer selectable...' being displayed on a non mod page is mentioned in MDL-28150 .
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Big thanks to Andrew and Helen! This is going to be the most-iterated issue ever, yay.

          I'm raising the 2 followups of this as blockers for next week: MDL-28149 and MDL-28150

          Ciao

          PS: I think we won't be backporting this to 20_STABLE, lol. Let's see.

          Show
          Eloy Lafuente (stronk7) added a comment - Big thanks to Andrew and Helen! This is going to be the most-iterated issue ever, yay. I'm raising the 2 followups of this as blockers for next week: MDL-28149 and MDL-28150 Ciao PS: I think we won't be backporting this to 20_STABLE, lol. Let's see.
          Hide
          Martin Dougiamas added a comment -

          And there was much rejoicing! Thanks to all on this!

          Show
          Martin Dougiamas added a comment - And there was much rejoicing! Thanks to all on this!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks Andrew and Helen! This was hard! Yay!

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks Andrew and Helen! This was hard! Yay!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: