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

          Attachments

            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