Moodle
  1. Moodle
  2. MDL-34992

unordered message providers settings options list thingy.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      43579

      Description

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

        Activity

        Hide
        Aparup Banerjee added a comment - - edited

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

        Show
        Aparup Banerjee added a comment - - edited patched - up for peer-review/feedback. Edit: should be easy backport.
        Hide
        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
        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
        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
        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
        Andrew Davis added a comment -

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

        Show
        Andrew Davis added a comment - If it all seems to work go ahead and submit it for integration
        Hide
        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
        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
        Michael de Raadt added a comment -

        Is this going to change the interface?

        Show
        Michael de Raadt added a comment - Is this going to change the interface?
        Hide
        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
        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
        Martin Dougiamas added a comment -

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

        Show
        Martin Dougiamas added a comment - I think it should sort by component and then group them with headings.
        Hide
        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
        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
        Aparup Banerjee added a comment -

        reopening: in the midst of adding groupings

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

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

        Show
        Michael de Raadt added a comment - Adding the ui_change label. This sounds like it will be notable in the release notes.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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 added a comment -

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

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

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

        Show
        Aparup Banerjee added a comment - thanks for that catch Sam. cleaning that up now..
        Hide
        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
        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
        Sam Hemelryk added a comment -

        Thanks Apu, has been integrated now

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

        Tested this in 2.4 only.

        This works as expected.

        Test passed.

        Show
        Rossiani Wijaya added a comment - Tested this in 2.4 only. This works as expected. Test passed.
        Hide
        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
        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
        Mary Cooch added a comment -

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

        Show
        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: