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

Add advanced option to repeat_elements

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      1. In order to test this, grab the extra commit https://github.com/timhunt/moodle/commit/MDL-37932_example (alternatively, make similar changes to another form of your choice that uses repeat elements).

      2. Go to the question bank and choose to create a Calculated question.

      3. There should be some advanced fields for each answer. Check that the Show more/less links work there.

      4. Check that saving the form still works, and that all the values are correctly saved. (e.g. re-edit the question).

      Show
      1. In order to test this, grab the extra commit https://github.com/timhunt/moodle/commit/MDL-37932_example (alternatively, make similar changes to another form of your choice that uses repeat elements). 2. Go to the question bank and choose to create a Calculated question. 3. There should be some advanced fields for each answer. Check that the Show more/less links work there. 4. Check that saving the form still works, and that all the values are correctly saved. (e.g. re-edit the question).
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      It is not currently possible to use an advanced option in repeat_elements (the only available option are default, helpbutton, disabledif, rule, type, expanded)
      having an advanced option would be quite useful and even more useful once MDL-30637 is integrated with the "sow more", "show less" feature for advanced fields.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            This works for me. However, it seems that it does not work for Jean-Michel, who is running the code from MDL-30637.

            I don't think we should delay getting MDL-30637 integrated just for this, but it would be nice to fix it later.

            Show
            timhunt Tim Hunt added a comment - This works for me. However, it seems that it does not work for Jean-Michel, who is running the code from MDL-30637 . I don't think we should delay getting MDL-30637 integrated just for this, but it would be nice to fix it later.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Yes it works well in master without MDL-30637 but break if MDL-30637 is applied.
            +1 to not stop integrating MDL-30637.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Yes it works well in master without MDL-30637 but break if MDL-30637 is applied. +1 to not stop integrating MDL-30637 .
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Tim,

            I've just cherry-picked this onto integration/master to see how it plays and I'm seeing JS issues whereby the hidden input element which stores the advanced state for the fieldset (mform_isexpanded_answerhdr[0]) is named differently to the fieldset it applies to (answerhdr_0).

            The JS currently uses the following to fetch the hidden input: var statuselement = new Y.one('input[name=mform_showmore_'+fieldset.get('id')+']');

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

            On the whole this looks like an easy change, but I guess there's more to be done given the above issues.

            Obviously there aren't any testing instructions here yet, but does this need developer documentation too?

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Tim, I've just cherry-picked this onto integration/master to see how it plays and I'm seeing JS issues whereby the hidden input element which stores the advanced state for the fieldset (mform_isexpanded_answerhdr [0] ) is named differently to the fieldset it applies to (answerhdr_0). The JS currently uses the following to fetch the hidden input: var statuselement = new Y.one('input [name=mform_showmore_'+fieldset.get('id')+'] '); [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [N] Testing [Y] Security [?] Documentation [Y] Git [Y] Sanity check On the whole this looks like an easy change, but I guess there's more to be done given the above issues. Obviously there aren't any testing instructions here yet, but does this need developer documentation too? Andrew
            Hide
            timhunt Tim Hunt added a comment -

            Andrew, did you see the bit above saying we needed to wait for MDL-30637 to land since that broke this? Yes. This needs more work.

            Show
            timhunt Tim Hunt added a comment - Andrew, did you see the bit above saying we needed to wait for MDL-30637 to land since that broke this? Yes. This needs more work.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Yes - hence I tested on integration/master which has MDL-30637.

            Show
            dobedobedoh Andrew Nicols added a comment - Yes - hence I tested on integration/master which has MDL-30637 .
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            hello Andrew,
            Thanks for working on this. It would be interesting for me (and other developers too I think) if this change could play nicely with MDL-30637.
            Do you see any solution to the JS issues you raised ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - hello Andrew, Thanks for working on this. It would be interesting for me (and other developers too I think) if this change could play nicely with MDL-30637 . Do you see any solution to the JS issues you raised ?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Jean-Michel Vedrine I'll admit I haven't looked. I was just providing a peer review of this issue now that MDL-30637 has been integrated.

            Show
            dobedobedoh Andrew Nicols added a comment - Jean-Michel Vedrine I'll admit I haven't looked. I was just providing a peer review of this issue now that MDL-30637 has been integrated.
            Hide
            timhunt Tim Hunt added a comment -

            OK, https://github.com/timhunt/moodle/compare/master...MDL-37932 is ready for review. This works for me, but I am not sure we are happy with the change to use id. Let me know what you think.

            Show
            timhunt Tim Hunt added a comment - OK, https://github.com/timhunt/moodle/compare/master...MDL-37932 is ready for review. This works for me, but I am not sure we are happy with the change to use id. Let me know what you think.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello,
            I can't answer the question if it's good or not to use id, but let me say that it works for me and that formulas questions parts look a lot better with this change.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello, I can't answer the question if it's good or not to use id, but let me say that it works for me and that formulas questions parts look a lot better with this change.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Linking the issue about advanced in repeat_elements and shortforms META

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Linking the issue about advanced in repeat_elements and shortforms META
            Hide
            dobedobedoh Andrew Nicols added a comment -

            New peer review:

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

            Just a few minor points, most of which you're almost certainly aware of already.

            Testing

            No testing instructions as yet

            Documentation

            lib/formslib.php: Could do with an comment explaining the reason by the str_replace on realelementid.

            Git

            Just needs a rebase to tidy up the steps along the way

            Otherwise looks good to go.

            Show
            dobedobedoh Andrew Nicols added a comment - New peer review: [Y] Syntax [Y] Output [Y] Whitespace [Y] Language [-] Databases [N] Testing [Y] Security [N] Documentation [N] Git [Y] Sanity check Just a few minor points, most of which you're almost certainly aware of already. Testing No testing instructions as yet Documentation lib/formslib.php: Could do with an comment explaining the reason by the str_replace on realelementid. Git Just needs a rebase to tidy up the steps along the way Otherwise looks good to go.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Andrew. Branch revised and submitted for integration.

            Show
            timhunt Tim Hunt added a comment - Thanks Andrew. Branch revised and submitted for integration.
            Hide
            timhunt Tim Hunt added a comment -

            Oops! I failed to click the Submit for integration button before. Really doing now.

            Show
            timhunt Tim Hunt added a comment - Oops! I failed to click the Submit for integration button before. Really doing now.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Tim, changes looked good and this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Tim, changes looked good and this has been integrated now.
            Hide
            andyjdavis Andrew Davis added a comment -

            I think this is working as described. Passing. I raised MDL-38363 as I encountered some PHP warnings.

            Show
            andyjdavis Andrew Davis added a comment - I think this is working as described. Passing. I raised MDL-38363 as I encountered some PHP warnings.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

            Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

            Thanks, closing as fixed!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

              People

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

                Dates

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