Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Portfolio API
    • Labels:
    • Testing Instructions:
      Hide

      This test requires the following:
      Portfolios and the Box.net, Flickr and Picasa portfolio plugins enabled for the site
      A forum activity containing a post by a student with an image attached
      Box.net, Flickr and Picasa accounts for use in testing
      1. Login as the student and access the forum post.
      2. Try exporting the post, selecting Box.net as the portfolio.
      3. Check that both the post and attachment are exported correctly to Box.net.
      4. Try exporting the attachment, selecting Flickr as the portfolio.
      5. Check that the image is exported correctly to Flickr.
      6. Try exporting the attachment again, selecting Picasa as the portfolio.
      7. Check that the image is exported correctly to Picasa.

      Show
      This test requires the following: Portfolios and the Box.net, Flickr and Picasa portfolio plugins enabled for the site A forum activity containing a post by a student with an image attached Box.net, Flickr and Picasa accounts for use in testing 1. Login as the student and access the forum post. 2. Try exporting the post, selecting Box.net as the portfolio. 3. Check that both the post and attachment are exported correctly to Box.net. 4. Try exporting the attachment, selecting Flickr as the portfolio. 5. Check that the image is exported correctly to Flickr. 6. Try exporting the attachment again, selecting Picasa as the portfolio. 7. Check that the image is exported correctly to Picasa.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      50040

      Description

      To replicate these warnings, follow the instructions of the linked QA test.

      The warnings in Box.net are...

      Did you remember to call setType() for 'plugin_newfolder'? Defaulting to PARAM_RAW cleaning.
      
      line 1289 of /lib/formslib.php: call to debugging()
      line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
      line 202 of /lib/formslib.php: call to moodleform->_process_submission()
      line 301 of /lib/portfolio/exporter.php: call to moodleform->moodleform()
      line 227 of /lib/portfolio/exporter.php: call to portfolio_exporter->process_stage_config()
      line 265 of /portfolio/add.php: call to portfolio_exporter->process_stage()
      

      This relates to the form elements in...

      portfolio/boxnet/lib.php, line 96
              $mform->addElement('text', 'plugin_newfolder', get_string('newfolder', 'portfolio_boxnet'));
      

      The warnings in Flickr are...

      Did you remember to call setType() for 'plugin_title'? Defaulting to PARAM_RAW cleaning.
      line 1289 of /lib/formslib.php: call to debugging()
      line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
      line 202 of /lib/formslib.php: call to moodleform->_process_submission()
      line 301 of /lib/portfolio/exporter.php: call to moodleform->moodleform()
      line 227 of /lib/portfolio/exporter.php: call to portfolio_exporter->process_stage_config()
      line 265 of /portfolio/add.php: call to portfolio_exporter->process_stage()
      
      Did you remember to call setType() for 'plugin_tags'? Defaulting to PARAM_RAW cleaning.
      line 1289 of /lib/formslib.php: call to debugging()
      line 281 of /lib/formslib.php: call to moodleform->detectMissingSetType()
      line 202 of /lib/formslib.php: call to moodleform->_process_submission()
      line 301 of /lib/portfolio/exporter.php: call to moodleform->moodleform()
      line 227 of /lib/portfolio/exporter.php: call to portfolio_exporter->process_stage_config()
      line 265 of /portfolio/add.php: call to portfolio_exporter->process_stage()
      

      This relates to the form elements in...

      porfolio/flickr/lib.php, lines 160-162
              $mform->addElement('text', 'plugin_title', get_string('title', 'portfolio_flickr'));
              $mform->addElement('textarea', 'plugin_description', get_string('description'));
              $mform->addElement('text', 'plugin_tags', get_string('tags'));
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          That looks good, Rosie.

          I'm wondering if we need to constrain the text for the Box.net folder name. As it's for a system outside of Moodle, we probably shouldn't, so PARAM_TEXT is fine.

          Show
          Michael de Raadt added a comment - That looks good, Rosie. I'm wondering if we need to constrain the text for the Box.net folder name. As it's for a system outside of Moodle, we probably shouldn't, so PARAM_TEXT is fine.
          Hide
          Michael de Raadt added a comment -

          Sending to integration...

          Show
          Michael de Raadt added a comment - Sending to integration...
          Hide
          Damyon Wiese added a comment -

          Hi Rosie,

          PARAM_TEXT makes no sense for plugin_newfolder or plugin_tags - please check the code and find a more appropriate type.

          Also - there is no setType for plugin_description in flickr.

          Can you update the patch please?

          Thanks!

          Show
          Damyon Wiese added a comment - Hi Rosie, PARAM_TEXT makes no sense for plugin_newfolder or plugin_tags - please check the code and find a more appropriate type. Also - there is no setType for plugin_description in flickr. Can you update the patch please? Thanks!
          Hide
          Rossiani Wijaya added a comment -

          Hi Damyon,

          Thanks for the feedback.

          I was thinking about multilang when fixing the issue. If I set the param to safedir/alphanumext, it might invalidate other characters that is not "a-zA-Z0-9_-".

          Although I agree with you that param_text doesn't seem right, but because of the multilang reason I decided to use text instead safedir/alphanumext.

          I will update the patch.

          Show
          Rossiani Wijaya added a comment - Hi Damyon, Thanks for the feedback. I was thinking about multilang when fixing the issue. If I set the param to safedir/alphanumext, it might invalidate other characters that is not "a-zA-Z0-9_-". Although I agree with you that param_text doesn't seem right, but because of the multilang reason I decided to use text instead safedir/alphanumext. I will update the patch.
          Hide
          Michael de Raadt added a comment -

          I'm not sure how much filtering we should apply to values sent to these external systems. I think that as long as they remain safe within Moodle, we should avoid trying to control them further.

          Show
          Michael de Raadt added a comment - I'm not sure how much filtering we should apply to values sent to these external systems. I think that as long as they remain safe within Moodle, we should avoid trying to control them further.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Rossiani Wijaya added a comment -

          Hi Damyon,

          I changed the following for setType() params:

          1. 'plugin_newfolder' - PARAM_NOTAGS. This will allowing other character to be used for naming the new folder (as we discussed earlier).
          2. 'plugin_description' - PARAM_CLEANHTML. I chose this instead of PARAM_TEXT because it will eliminate script input for description. I think PARAM_TEXT is more for our advantage instead of external site.
          3. 'plugin_tags' - PARAM_TAG. This will make the tag name safer to display. So I decided to use this instead of PARAM_NOTAGS.

          So Let me know what do you think about this changes.

          Patch updated.

          Show
          Rossiani Wijaya added a comment - Hi Damyon, I changed the following for setType() params: 'plugin_newfolder' - PARAM_NOTAGS. This will allowing other character to be used for naming the new folder (as we discussed earlier). 'plugin_description' - PARAM_CLEANHTML. I chose this instead of PARAM_TEXT because it will eliminate script input for description. I think PARAM_TEXT is more for our advantage instead of external site. 'plugin_tags' - PARAM_TAG. This will make the tag name safer to display. So I decided to use this instead of PARAM_NOTAGS. So Let me know what do you think about this changes. Patch updated.
          Hide
          Rossiani Wijaya added a comment -

          Resubmitting for integration review.

          Show
          Rossiani Wijaya added a comment - Resubmitting for integration review.
          Hide
          Damyon Wiese added a comment -

          Hi Rosie - some more clarification:

          PARAM_TEXT does clean all tags except multilang - so this would be better that CLEANHTML for plugin_description.

          PARAM_TAG will not allow , - so this will only allow a single tag - it should be PARAM_TAGLIST

          plugin_newfolder is OK with NOTAGS - but RAW would have been fine here too I think - we should rely on the external system to clean it's own params.

          Show
          Damyon Wiese added a comment - Hi Rosie - some more clarification: PARAM_TEXT does clean all tags except multilang - so this would be better that CLEANHTML for plugin_description. PARAM_TAG will not allow , - so this will only allow a single tag - it should be PARAM_TAGLIST plugin_newfolder is OK with NOTAGS - but RAW would have been fine here too I think - we should rely on the external system to clean it's own params.
          Hide
          Rossiani Wijaya added a comment -

          Patch updated.

          Show
          Rossiani Wijaya added a comment - Patch updated.
          Hide
          Damyon Wiese added a comment -

          Thanks Rosie,

          This has been integrated to master now.

          Show
          Damyon Wiese added a comment - Thanks Rosie, This has been integrated to master now.
          Hide
          Dan Poltawski added a comment -

          I tested it and it passes. I didn't test picasa as this wasn't affected by this bug/patch.

          Show
          Dan Poltawski added a comment - I tested it and it passes. I didn't test picasa as this wasn't affected by this bug/patch.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao
          Hide
          Michael de Raadt added a comment -

          There was no need to test this issue. The testing is covered by the associated QA test (which at this point is still open).

          Show
          Michael de Raadt added a comment - There was no need to test this issue. The testing is covered by the associated QA test (which at this point is still open).
          Hide
          Andrew Davis added a comment -

          Picasa and box.net are now working fine. There's something going on with Flickr but thats being dealt with in MDL-39479.

          Show
          Andrew Davis added a comment - Picasa and box.net are now working fine. There's something going on with Flickr but thats being dealt with in MDL-39479 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: