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

unordered message providers settings options list thingy.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Messages
    • Labels:
    • Testing Instructions:
      Hide

      look at your profile settings > messaging screen.

      check that the sequence of message providers makes more sense than before.

      try with a number of components or (contributed) plugins that have a db/messages.php file if possible.

      namely:
      1) check that the settings are grouped into component tables.
      2) check that the disable all notifications still works (since it was moved into another field set)
      3) play around with messaging settings and see that the providers and settings make sense as they did before. (email messaging, popup .. jabber even?)

      Show
      look at your profile settings > messaging screen. check that the sequence of message providers makes more sense than before. try with a number of components or (contributed) plugins that have a db/messages.php file if possible. namely: 1) check that the settings are grouped into component tables. 2) check that the disable all notifications still works (since it was moved into another field set) 3) play around with messaging settings and see that the providers and settings make sense as they did before. (email messaging, popup .. jabber even?)
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      The messaging options list looks chaotic - here's a quick patch.

        Gliffy Diagrams

          Activity

          Hide
          nebgor Aparup Banerjee added a comment - - edited

          patched - up for peer-review/feedback.
          Edit: should be easy backport.

          Show
          nebgor Aparup Banerjee added a comment - - edited patched - up for peer-review/feedback. Edit: should be easy backport.
          Hide
          andyjdavis Andrew Davis added a comment -

          Whats the reason for sorting by array element then by array key? It seems like doing the second sort is going to make the first sort pointless...

          Show
          andyjdavis Andrew Davis added a comment - Whats the reason for sorting by array element then by array key? It seems like doing the second sort is going to make the first sort pointless...
          Hide
          nebgor Aparup Banerjee added a comment -

          ah, it sorts the values then the keys , in this way hoping the values have some sequence too once the keys are sequenced. i thought it did that from a quick check.

          Show
          nebgor Aparup Banerjee added a comment - ah, it sorts the values then the keys , in this way hoping the values have some sequence too once the keys are sequenced. i thought it did that from a quick check.
          Hide
          andyjdavis Andrew Davis added a comment -

          If it all seems to work go ahead and submit it for integration

          Show
          andyjdavis Andrew Davis added a comment - If it all seems to work go ahead and submit it for integration
          Hide
          nebgor Aparup Banerjee added a comment -

          up for further scrutiny.

          nb: could be made a little better by providing a grouping in the form based on component_*

          Show
          nebgor Aparup Banerjee added a comment - up for further scrutiny. nb: could be made a little better by providing a grouping in the form based on component_*
          Hide
          salvetore Michael de Raadt added a comment -

          Is this going to change the interface?

          Show
          salvetore Michael de Raadt added a comment - Is this going to change the interface?
          Hide
          nebgor Aparup Banerjee added a comment -

          hrm strictly speaking , yes the ui does change. However, its a simple re-ordering of rows of checkboxes - so functionally no ui_change. its rather a aesthetic ui change. tag it so if you want to be safe

          Show
          nebgor Aparup Banerjee added a comment - hrm strictly speaking , yes the ui does change. However, its a simple re-ordering of rows of checkboxes - so functionally no ui_change. its rather a aesthetic ui change. tag it so if you want to be safe
          Hide
          dougiamas Martin Dougiamas added a comment -

          I think it should sort by component and then group them with headings.

          Show
          dougiamas Martin Dougiamas added a comment - I think it should sort by component and then group them with headings.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

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

          TIA and ciao

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

          reopening: in the midst of adding groupings

          Show
          nebgor Aparup Banerjee added a comment - reopening: in the midst of adding groupings
          Hide
          salvetore Michael de Raadt added a comment -

          Adding the ui_change label. This sounds like it will be notable in the release notes.

          Show
          salvetore Michael de Raadt added a comment - Adding the ui_change label. This sounds like it will be notable in the release notes.
          Hide
          nebgor Aparup Banerjee added a comment - - edited

          ok, grouped into components.
          It was a really long list of tables so i made them float (see screenshot)
          i'd like to make the provider name span nicely without making the tables seems different widths but got no ideas there so submitting for peer-review

          ps: note that i've moved the 'disable notifications' setting into under 'general' field set as it seems not so cluttered then and actually seems to belong in general too.

          Show
          nebgor Aparup Banerjee added a comment - - edited ok, grouped into components. It was a really long list of tables so i made them float (see screenshot) i'd like to make the provider name span nicely without making the tables seems different widths but got no ideas there so submitting for peer-review ps: note that i've moved the 'disable notifications' setting into under 'general' field set as it seems not so cluttered then and actually seems to belong in general too.
          Hide
          itamart Itamar Tzadok added a comment -

          This solution may be effective only for admin who can see all providers. Ordinary users can't see providers that are constrained by capability in module context (and possibly in course context). See MDL-35582. Generating the providers list for all context levels per the given user may be extremely expensive so the ui should probably include designated navigation/filter for displaying providers by context.

          Show
          itamart Itamar Tzadok added a comment - This solution may be effective only for admin who can see all providers. Ordinary users can't see providers that are constrained by capability in module context (and possibly in course context). See MDL-35582 . Generating the providers list for all context levels per the given user may be extremely expensive so the ui should probably include designated navigation/filter for displaying providers by context.
          Hide
          nebgor Aparup Banerjee added a comment -

          Thanks for looking at this Itamar.
          You're patch in MDL-35582 seems to be headed in the correct direction from a glance but i'll let Andrew review it there.
          I understand that this doesn't affect this patch being integrated as this is about aesthetics.

          Show
          nebgor Aparup Banerjee added a comment - Thanks for looking at this Itamar. You're patch in MDL-35582 seems to be headed in the correct direction from a glance but i'll let Andrew review it there. I understand that this doesn't affect this patch being integrated as this is about aesthetics.
          Hide
          andyjdavis Andrew Davis added a comment - - edited

          [Y] Syntax
          [Y] Output
          [N] Whitespace
          [Y] Language
          [Y] Databases
          [N] Testing
          [Y] Security
          [NA] Documentation
          [N] Git
          [Y] Sanity check

          "$table->data = array();"
          line 244 appears to be overly indentified.

          The component in your commit messages is inconsistent and incorrect in both cases. I believe it should be core_message.

          In the testing instructions include a summary of how the grouping should work. I'd also include actually getting the tester to modify their settings just to make absolutely sure its all fine.

          All pretty minor.

          Show
          andyjdavis Andrew Davis added a comment - - edited [Y] Syntax [Y] Output [N] Whitespace [Y] Language [Y] Databases [N] Testing [Y] Security [NA] Documentation [N] Git [Y] Sanity check "$table->data = array();" line 244 appears to be overly indentified. The component in your commit messages is inconsistent and incorrect in both cases. I believe it should be core_message. In the testing instructions include a summary of how the grouping should work. I'd also include actually getting the tester to modify their settings just to make absolutely sure its all fine. All pretty minor.
          Hide
          nebgor Aparup Banerjee added a comment -

          Thanks for that check(list) Andrew
          i've cleaned up line 244 as well as 250's indents.
          i've gone with 'message' as the code area which is prevalent from commit logs. (i saw 'core_message' only in AMOS commands)

          sending this to integrators.

          Show
          nebgor Aparup Banerjee added a comment - Thanks for that check(list) Andrew i've cleaned up line 244 as well as 250's indents. i've gone with 'message' as the code area which is prevalent from commit logs. (i saw 'core_message' only in AMOS commands) sending this to integrators.
          Hide
          poltawski Dan Poltawski added a comment -

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

          TIA and ciao

          Show
          poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Apu,

          Reopening this sorry dude, there was just one thing that I spotted in this code that is a problem.
          The sorting of components in renderer.php breaks horribly as soon as you have an admin tool, theme, or any other plugin with a component starting with a letter greater than M that also has a messages.php.
          The code removes the last plugin and adds Moodle back instead leading to the system settings being shown twice (Simple way to reproduce is to add a messages.php file to an admin tool).
          Potentially you could argue that we don't have any such plugins in core that have a messages.php file, however I could imagine admin tools in particular sending messages and I think that use case is pretty strong and that we should clean this up now while we are aware of it.

          Perhaps while you're there any way you wouldn't mind cleaning up the underscore in $number_procs, I see it was there originally, however there is just its initialisation and a single use both lines that you've modified so would be a good time to clean it up.

          Many thanks
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Apu, Reopening this sorry dude, there was just one thing that I spotted in this code that is a problem. The sorting of components in renderer.php breaks horribly as soon as you have an admin tool, theme, or any other plugin with a component starting with a letter greater than M that also has a messages.php. The code removes the last plugin and adds Moodle back instead leading to the system settings being shown twice (Simple way to reproduce is to add a messages.php file to an admin tool). Potentially you could argue that we don't have any such plugins in core that have a messages.php file, however I could imagine admin tools in particular sending messages and I think that use case is pretty strong and that we should clean this up now while we are aware of it. Perhaps while you're there any way you wouldn't mind cleaning up the underscore in $number_procs, I see it was there originally, however there is just its initialisation and a single use both lines that you've modified so would be a good time to clean it up. Many thanks Sam
          Hide
          cibot CiBoT added a comment -

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

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

          thanks for that catch Sam. cleaning that up now..

          Show
          nebgor Aparup Banerjee added a comment - thanks for that catch Sam. cleaning that up now..
          Hide
          nebgor Aparup Banerjee added a comment -

          That dodgy sorting has been reduced by one array action now. Code cleaned up as suggested too.
          Appended test to include various plugins with db/messages.php if possible.

          All yours

          Show
          nebgor Aparup Banerjee added a comment - That dodgy sorting has been reduced by one array action now. Code cleaned up as suggested too. Appended test to include various plugins with db/messages.php if possible. All yours
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Apu, has been integrated now

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Apu, has been integrated now
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Tested this in 2.4 only.

          This works as expected.

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - Tested this in 2.4 only. This works as expected. Test passed.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          From somewhere within the clouds...

          Congrats, this has been sent upstream and is now part of Moodle (your favorite LMS platform). Many thanks for your awesome collaboration!

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - From somewhere within the clouds... Congrats, this has been sent upstream and is now part of Moodle (your favorite LMS platform). Many thanks for your awesome collaboration! Ciao
          Hide
          marycooch Mary Cooch added a comment -

          Removing docs_required label as this is documented here http://docs.moodle.org/24/en/Messaging_settings

          Show
          marycooch Mary Cooch added a comment - Removing docs_required label as this is documented here http://docs.moodle.org/24/en/Messaging_settings

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                3/Dec/12