Moodle
  1. Moodle
  2. MDL-33828

Exporting assignment 2.3 to portfolios Flickr & Box.net

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3.1
    • Component/s: Assignment, Portfolio
    • Labels:
    • 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

      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

        Gliffy Diagrams

          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: