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:
    • Rank:
      47691

      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.

        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: