Moodle
  1. Moodle
  2. MDL-33828

Exporting assignment 2.3 to portfolios Flickr & Box.net

    Details

    • Testing Instructions:
      Hide
      1. Enable Flickr and Box.net portfolios
      2. Create a standard assignment 2.3
      3. Login as a student and submit an image as assignment
      4. Export to Flickr and Box.net
      5. Ensure no notices or warnings appear during the whole process
      Show
      Enable Flickr and Box.net portfolios Create a standard assignment 2.3 Login as a student and submit an image as assignment Export to Flickr and Box.net Ensure no notices or warnings appear during the whole process
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33828-master
    • Rank:
      41927

      Description

      1. Enable Flickr and Box.net portfolios
      2. Create a standard assignment 2.3
      3. Login as a student and submit an image as assignment
      4. Try to export the image to portfolio

      Actual result:

      Exporting to Box.net or Flickr throws a Javascript alert "If parent window is on HTTPS, then we are not allowed to access window.opener object, so we cannot refresh the repository for you automatically, but we already got your session, just go back to file picker and select the repository again, it should work now.". There might be a way of having this working but I did not find one. I went back, I started again, ... did not work.

      I also noticed that once this export failed, I could not choose Flickr or Box.net in the portfolio dropdown menu.

      This noticed appeared during the export attempt:

      Strict Standards: Only variables should be passed by reference in /home/fred/www/repositories/testing_master/moodle/lib/portfolio/exporter.php on line 727
      

        Issue Links

          Activity

          Hide
          Damyon Wiese added a comment -

          This appears to be an issue with the repository plugin / box.net and flicker repository plugins.

          Assigning back to moodle.com

          Show
          Damyon Wiese added a comment - This appears to be an issue with the repository plugin / box.net and flicker repository plugins. Assigning back to moodle.com
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          I think you've got some leftover code from another issue here (adding $component etc).

          Could you explain more about what the change was and why its necessary?

          Show
          Dan Poltawski added a comment - Hi Fred, I think you've got some leftover code from another issue here (adding $component etc). Could you explain more about what the change was and why its necessary?
          Hide
          Frédéric Massart added a comment -

          Oh, I forgot to comment on my work... First of all, I could not reproduce the Javascript alert. I guess it has been fixed or modified in the meantime. But I came across 3 notices.

          There is the one in the description of this issue, which is the same than the second one (change in boxnet). The third notice happened on this line (https://github.com/FMCorz/moodle/blob/b32224055dacf4c7641fff8d9f93b5cbbead8f85/mod/assign/portfolio_callback.php#L114) because $component was not set.

          Show
          Frédéric Massart added a comment - Oh, I forgot to comment on my work... First of all, I could not reproduce the Javascript alert. I guess it has been fixed or modified in the meantime. But I came across 3 notices. There is the one in the description of this issue, which is the same than the second one (change in boxnet). The third notice happened on this line ( https://github.com/FMCorz/moodle/blob/b32224055dacf4c7641fff8d9f93b5cbbead8f85/mod/assign/portfolio_callback.php#L114 ) because $component was not set.
          Hide
          Dan Poltawski added a comment -

          Thanks Fred - pushing for integration.

          Show
          Dan Poltawski added a comment - Thanks Fred - pushing for integration.
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          I've just been looking at this now.
          Before this gets in I think the order of the checks in exporter.php needs to be looked at.
          Looking before and after a couple of things strike me as having been done for performance.

          1. Really the first check should be !$readonly. Its a bool and delivered as an arg so available from the start. If its false theres no point in checking anything else.
          2. Second check should be $this->get('instance') (currently first) thats an easy check.
          3. allows_multiple_exports next, thats a simple method that returns a bool.

          Those three checks are simple and easy, they should really be made first as if any fails we avoid the more complex comparisons.
          After that all that is left is to make the key check.
          The original order of the checks reflected this but it appears to have been lost in your changes.

          Could you please fix up the order of those checks and ping me when done. I'll leave this as in review so that it can still be done if you get it up in time.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, I've just been looking at this now. Before this gets in I think the order of the checks in exporter.php needs to be looked at. Looking before and after a couple of things strike me as having been done for performance. Really the first check should be !$readonly. Its a bool and delivered as an arg so available from the start. If its false theres no point in checking anything else. Second check should be $this->get('instance') (currently first) thats an easy check. allows_multiple_exports next, thats a simple method that returns a bool. Those three checks are simple and easy, they should really be made first as if any fails we avoid the more complex comparisons. After that all that is left is to make the key check. The original order of the checks reflected this but it appears to have been lost in your changes. Could you please fix up the order of those checks and ping me when done. I'll leave this as in review so that it can still be done if you get it up in time. Cheers Sam
          Hide
          Frédéric Massart added a comment -

          You made a very good point here, I did not realize that I had changed that. I have updated my branch. Thank you Sam!

          Show
          Frédéric Massart added a comment - You made a very good point here, I did not realize that I had changed that. I have updated my branch. Thank you Sam!
          Hide
          Sam Hemelryk added a comment -

          Thanks Fred has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Fred has been integrated now
          Hide
          Tim Barker added a comment -

          The test passes! Awesome work. We did potentially find another issue while testing which we will raise elsewhere.

          Show
          Tim Barker added a comment - The test passes! Awesome work. We did potentially find another issue while testing which we will raise elsewhere.
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: