Moodle
  1. Moodle
  2. MDL-37932

Add advanced option to repeat_elements

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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
            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
            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
            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
            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
            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
            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
            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
            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
            Andrew Nicols added a comment -

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

            Show
            Andrew Nicols added a comment - Yes - hence I tested on integration/master which has MDL-30637 .
            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            Jean-Michel Vedrine added a comment -

            Linking the issue about advanced in repeat_elements and shortforms META

            Show
            Jean-Michel Vedrine added a comment - Linking the issue about advanced in repeat_elements and shortforms META
            Hide
            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
            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
            Tim Hunt added a comment -

            Thanks Andrew. Branch revised and submitted for integration.

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

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

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

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

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

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

            Show
            Andrew Davis added a comment - I think this is working as described. Passing. I raised MDL-38363 as I encountered some PHP warnings.
            Hide
            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
            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: