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

it is not possible to add a picture to the thanks page in feedback

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.5, 2.2.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Feedback
    • Labels:
    • Testing Instructions:
      Hide
      1. create a new feedback instance
      2. insert a picture into the page_after_submit editor field
      3. save the new instance
      4. update the instance
      5. add another picture into the page_after_submit editor field
      6. save the instance
      7. open the feedback instance. The pictures should be shown at the overview page
      8. add one or more questions
      9. complete this feedback. At the end the page_after_submit page should be shown
      10. backup the course including the feedback instance
      11. restore the course including the feedback instance
      12. doublicate the feedback instance inside the course
      Show
      create a new feedback instance insert a picture into the page_after_submit editor field save the new instance update the instance add another picture into the page_after_submit editor field save the instance open the feedback instance. The pictures should be shown at the overview page add one or more questions complete this feedback. At the end the page_after_submit page should be shown backup the course including the feedback instance restore the course including the feedback instance doublicate the feedback instance inside the course
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31768_master_wip

      Description

      It is not possible to upload a picture in the "Page after submit" textarea of the feedback settings page.

        Gliffy Diagrams

          Activity

          Hide
          grabs Andreas Grabs added a comment -

          Hi Danielle,
          thank you for reporting this.
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Danielle, thank you for reporting this. Andreas
          Hide
          nebgor Aparup Banerjee added a comment -

          I'm not really sure this is a bug - its definitely not been implemented before but I'm happy to put this into master.

          Show
          nebgor Aparup Banerjee added a comment - I'm not really sure this is a bug - its definitely not been implemented before but I'm happy to put this into master.
          Hide
          nebgor Aparup Banerjee added a comment -

          Hi Andreas,
          i've found some whitespaces and code formatting issues which are minor but i'm short on time today.

          also, please send this issue up for peer-review , it helps that there are more eyes looking at the code going into core

          (still thinking its going into master only tho, will look at this next week)

          Show
          nebgor Aparup Banerjee added a comment - Hi Andreas, i've found some whitespaces and code formatting issues which are minor but i'm short on time today. also, please send this issue up for peer-review , it helps that there are more eyes looking at the code going into core (still thinking its going into master only tho, will look at this next week)
          Hide
          grabs Andreas Grabs added a comment -

          Hi Aparup,
          I do not understand, what I have to do. How can I send this issue up for peer-review? What is this meaning?
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Aparup, I do not understand, what I have to do. How can I send this issue up for peer-review? What is this meaning? Andreas
          Hide
          nebgor Aparup Banerjee added a comment -

          Hi Andreas,
          i think the work done is great but i think its actually a new function right? not really a bug - so its better that this goes only into master (instead of changing things in STABLE versions).

          besides that i noticed this issue wasn't peer-reviewed. we've got a peer-review process described @ http://docs.moodle.org/dev/Process - basically after development it helps to get the issue peer reviewed. so in the end at least 3 eyes have gone through the code (1:developer ,2: peer-reviewer, 3: integrator)

          Ah, i'm guessing you probably can't see the 'peer-review' button ? It might be under the workflow menu for you among the 'send for integration group of buttons. We really should have images/docs for this in the process doc.

          This way we can reduce bugs/regressions and keep moodle quality high

          ps: i can fix this up on friday and get it integrated soon - i ran out of time. Just wanted to let you know about 'peer-review' stage in the workflow - cheers!

          Show
          nebgor Aparup Banerjee added a comment - Hi Andreas, i think the work done is great but i think its actually a new function right? not really a bug - so its better that this goes only into master (instead of changing things in STABLE versions). besides that i noticed this issue wasn't peer-reviewed. we've got a peer-review process described @ http://docs.moodle.org/dev/Process - basically after development it helps to get the issue peer reviewed. so in the end at least 3 eyes have gone through the code (1:developer ,2: peer-reviewer, 3: integrator) Ah, i'm guessing you probably can't see the 'peer-review' button ? It might be under the workflow menu for you among the 'send for integration group of buttons. We really should have images/docs for this in the process doc . This way we can reduce bugs/regressions and keep moodle quality high ps: i can fix this up on friday and get it integrated soon - i ran out of time. Just wanted to let you know about 'peer-review' stage in the workflow - cheers!
          Hide
          grabs Andreas Grabs added a comment -

          Hi Aparup,
          sorry, my english is not so good.
          If I understand you right I have to go to a button "peer-review". But I can't see such button. I did never need this before . Where can I do this?
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Aparup, sorry, my english is not so good. If I understand you right I have to go to a button "peer-review". But I can't see such button. I did never need this before . Where can I do this? Andreas
          Hide
          nebgor Aparup Banerjee added a comment - - edited

          Hi Andreas, please check with Helen or Michael (or MD?) - you should be able to see the peer-review button as a developer.

          edit: ah! the 'peer-review' option is only visible when the issue is in development. i'll push it along now to in-development so that you can see the 'peer-review'.

          Show
          nebgor Aparup Banerjee added a comment - - edited Hi Andreas, please check with Helen or Michael (or MD?) - you should be able to see the peer-review button as a developer. edit: ah! the 'peer-review' option is only visible when the issue is in development. i'll push it along now to in-development so that you can see the 'peer-review'.
          Hide
          nebgor Aparup Banerjee added a comment - - edited

          Andreas, its now in 'development in progress', you should be able to request 'peer-review' now. i can see it. i'll leave that bit to you to get familiar with

          Show
          nebgor Aparup Banerjee added a comment - - edited Andreas, its now in 'development in progress', you should be able to request 'peer-review' now. i can see it. i'll leave that bit to you to get familiar with
          Hide
          daniss Daniele Cordella added a comment -

          Me? Was it a distraction?

          Show
          daniss Daniele Cordella added a comment - Me? Was it a distraction?
          Hide
          grabs Andreas Grabs added a comment -

          Ok, now I start peer review. But I do not know who I should select for "Peer reviewer".
          Andreas

          Show
          grabs Andreas Grabs added a comment - Ok, now I start peer review. But I do not know who I should select for "Peer reviewer". Andreas
          Hide
          nebgor Aparup Banerjee added a comment - - edited

          Gal that its working for you Andreas

          it will now appear in a list for HQ to review (its optional to assign a peer-reviewer)

          Show
          nebgor Aparup Banerjee added a comment - - edited Gal that its working for you Andreas it will now appear in a list for HQ to review (its optional to assign a peer-reviewer)
          Hide
          grabs Andreas Grabs added a comment -

          Hi Aparup,

          there is still another question. Why do you say it is not a bug? Anybody who use an editor is expecting this function. I think it is a very important thing and should not wait until 2.3 is coming out. At least for 2.2 it should be go in.

          Best regards Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Aparup, there is still another question. Why do you say it is not a bug? Anybody who use an editor is expecting this function. I think it is a very important thing and should not wait until 2.3 is coming out. At least for 2.2 it should be go in. Best regards Andreas
          Hide
          grabs Andreas Grabs added a comment -

          Hi,
          I would like to know whether there are some things I can/have to do so that these commits will be integrated.
          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi, I would like to know whether there are some things I can/have to do so that these commits will be integrated. Best regards Andreas
          Hide
          nebgor Aparup Banerjee added a comment -

          Hi Andreas, i'm sending this up for integration now.

          I'm sure its expected to be able to put up pictures in "Page after submit". I just meant it wasn't implemented at all, its great it will be in there soon!

          Show
          nebgor Aparup Banerjee added a comment - Hi Andreas, i'm sending this up for integration now. I'm sure its expected to be able to put up pictures in "Page after submit". I just meant it wasn't implemented at all, its great it will be in there soon!
          Hide
          poltawski Dan Poltawski added a comment -

          The pre-checker has found a few coding style problems:

          <description>
          This sections shows the coding style problems detected in the code by phpcs
          </description>
          <mess>
          <problem file="mod/feedback/lib.php" linefrom="736" lineto="736" method="" class="" package="" api="" ruleset="Squiz" rule="Commenting.DocCommentAlignment.SpaceBeforeTag" url="http://docs.moodle.org/dev/Coding_style" weight="5">
          <message>Expected 1 space between asterisk and tag; 2 found</message>
          <description/>
          <code/>
          </problem>
          <problem file="mod/feedback/mod_form.php" linefrom="172" lineto="172" method="" class="" package="" api="" ruleset="Squiz" rule="WhiteSpace.SuperfluousWhitespace.EndLine" url="http://docs.moodle.org/dev/Coding_style" weight="5">
          <message>Whitespace found at end of line</message>
          <description/>
          <code/>
          </problem>
          <problem file="mod/feedback/mod_form.php" linefrom="178" lineto="178" method="" class="" package="" api="" ruleset="Squiz" rule="WhiteSpace.SuperfluousWhitespace.EndLine" url="http://docs.moodle.org/dev/Coding_style" weight="5">
          <message>Whitespace found at end of line</message>
          <description/>
          <code/>
          </problem>
          <problem file="mod/feedback/mod_form.php" linefrom="189" lineto="189" method="" class="" package="" api="" ruleset="moodle" rule="Files.LineLength.TooLong" url="http://docs.moodle.org/dev/Coding_style" weight="1">
          <message>
          Line exceeds 132 characters; contains 138 characters
          </message>
          <description/>
          <code/>
          </problem>

          Show
          poltawski Dan Poltawski added a comment - The pre-checker has found a few coding style problems: <description> This sections shows the coding style problems detected in the code by phpcs </description> <mess> <problem file="mod/feedback/lib.php" linefrom="736" lineto="736" method="" class="" package="" api="" ruleset="Squiz" rule="Commenting.DocCommentAlignment.SpaceBeforeTag" url="http://docs.moodle.org/dev/Coding_style" weight="5"> <message>Expected 1 space between asterisk and tag; 2 found</message> <description/> <code/> </problem> <problem file="mod/feedback/mod_form.php" linefrom="172" lineto="172" method="" class="" package="" api="" ruleset="Squiz" rule="WhiteSpace.SuperfluousWhitespace.EndLine" url="http://docs.moodle.org/dev/Coding_style" weight="5"> <message>Whitespace found at end of line</message> <description/> <code/> </problem> <problem file="mod/feedback/mod_form.php" linefrom="178" lineto="178" method="" class="" package="" api="" ruleset="Squiz" rule="WhiteSpace.SuperfluousWhitespace.EndLine" url="http://docs.moodle.org/dev/Coding_style" weight="5"> <message>Whitespace found at end of line</message> <description/> <code/> </problem> <problem file="mod/feedback/mod_form.php" linefrom="189" lineto="189" method="" class="" package="" api="" ruleset="moodle" rule="Files.LineLength.TooLong" url="http://docs.moodle.org/dev/Coding_style" weight="1"> <message> Line exceeds 132 characters; contains 138 characters </message> <description/> <code/> </problem>
          Hide
          grabs Andreas Grabs added a comment -

          Hi Dan,

          I have fixed the shown errors at master and 2.2.
          There still are some warnings but I think it makes no sense to shorten the related lines.

          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Dan, I have fixed the shown errors at master and 2.2. There still are some warnings but I think it makes no sense to shorten the related lines. Best regards Andreas
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Andreas,

          I notice that you are using itemid for this with the feedback id. Its not necessary to do this as the context of the filarea will link the item back to the feedback id in the main mod_feedback table.

          It'll be clearer if the itemid is not used as in this case its not necessary (the files are shared throughout the the whole feeback activity). So could you switch to not using the itemid. See the workfshop module for an example of that - with instructauthors.

          Thanks!
          dan

          Show
          poltawski Dan Poltawski added a comment - Hi Andreas, I notice that you are using itemid for this with the feedback id. Its not necessary to do this as the context of the filarea will link the item back to the feedback id in the main mod_feedback table. It'll be clearer if the itemid is not used as in this case its not necessary (the files are shared throughout the the whole feeback activity). So could you switch to not using the itemid. See the workfshop module for an example of that - with instructauthors. Thanks! dan
          Hide
          grabs Andreas Grabs added a comment -

          Hi Dan,

          ok, now I understand it better a bit .
          I have fixed these things.

          Best regards
          Andreas

          Show
          grabs Andreas Grabs added a comment - Hi Dan, ok, now I understand it better a bit . I have fixed these things. Best regards Andreas
          Hide
          stronk7 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
          stronk7 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
          poltawski Dan Poltawski added a comment -

          Thanks, that has been integrated now

          Show
          poltawski Dan Poltawski added a comment - Thanks, that has been integrated now
          Hide
          rwijaya Rossiani Wijaya added a comment -

          This looks good

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - This looks good Test passed.
          Hide
          nebgor Aparup Banerjee added a comment -

          The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

          Closing, have a good weekend!

          Show
          nebgor Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!

            People

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

              Dates

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