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

          Attachments

            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