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

Editing cohort description does not allow images of media to be uploaded

    Details

    • Testing Instructions:
      Hide

      As manager with capability to view and manage cohorts (but not as admin!):

      • Create cohort, add image in the description
      • Make sure it is displayed
      • Edit cohort, change the context
      • Make sure the image is still displayed on the view cohorts list
      • Edit cohort, change/add the image
      • Make sure the changes are saved and cohort is displayed correctly.
      • Copy image URL
      • Make sure user with only cohort/view capability can access URL
      • Make sure guest or user without cohort/view capability can not access URL
      Show
      As manager with capability to view and manage cohorts (but not as admin!): Create cohort, add image in the description Make sure it is displayed Edit cohort, change the context Make sure the image is still displayed on the view cohorts list Edit cohort, change/add the image Make sure the changes are saved and cohort is displayed correctly. Copy image URL Make sure user with only cohort/view capability can access URL Make sure guest or user without cohort/view capability can not access URL
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_30_STABLE
    • Pull Master Branch:
      wip-MDL-28954-master

      Description

      When editing the description of a cohort with the HTML editor, it is not possible to upload images or media.

      This may be because of the context not being set for cohorts (or being set to zero), so there is no temporary editing area for attached files.

      Replication steps:

      1. Log in as admin
      2. Navigate to Site Administration -> Users -> Accounts -> Cohorts
      3. Click on the "Insert/edit image" button or the "Moodle media" button.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Hello. I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

              For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

              If you have any information about this issue or a possible fix please post it here

              Show
              rajeshtaneja Rajesh Taneja added a comment - Hello. I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment If you have any information about this issue or a possible fix please post it here
              Hide
              marina Marina Glancy added a comment -

              Up for peer review. Note that this branch is based on MDL-50191

              Show
              marina Marina Glancy added a comment - Up for peer review. Note that this branch is based on MDL-50191
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-28954 using repository: https://github.com/marinaglancy/moodle

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28954 using repository: https://github.com/marinaglancy/moodle MOODLE_29_STABLE (2 errors / 2 warnings) [branch: wip-MDL-28954-m29 | CI Job ] phplint (0/0) , php (1/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
              Hide
              lameze Simey Lameze added a comment -

              Hi Marina, thanks for work on this.

              1. I've noticed the you've added the same changes to the cohort/index.php here, is there any reason for that?
                I know this issue is blocked by the MDL-50191, but I think the patch should be rebased on top of those changes.
              2. Another point is few calls like this:

                cohort_update_cohort((object)array('id' => $data->id, 'description' => $data->description, 'contextid' => $context->id)); 
                

                Not against declare and pass an array as parameter, I'm not a great fan of forcing it as (object) there. Up to you.

              Rest looks good. Feel free to push for integration.

              Thanks.

              Show
              lameze Simey Lameze added a comment - Hi Marina, thanks for work on this. I've noticed the you've added the same changes to the cohort/index.php here, is there any reason for that? I know this issue is blocked by the MDL-50191 , but I think the patch should be rebased on top of those changes. Another point is few calls like this: cohort_update_cohort((object)array('id' => $data->id, 'description' => $data->description, 'contextid' => $context->id)); Not against declare and pass an array as parameter, I'm not a great fan of forcing it as (object) there. Up to you. Rest looks good. Feel free to push for integration. Thanks.
              Hide
              marina Marina Glancy added a comment -

              Thanks Simey, I change the (object)

              TO INTEGRATOR: please note that the first commit in the branches is from another issue that is blocking this

              Show
              marina Marina Glancy added a comment - Thanks Simey, I change the (object) TO INTEGRATOR: please note that the first commit in the branches is from another issue that is blocking this
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28954 using repository: https://github.com/marinaglancy/moodle MOODLE_28_STABLE (2 errors / 2 warnings) [branch: wip-MDL-28954-m28 | CI Job ] phplint (0/0) , php (1/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_29_STABLE (2 errors / 2 warnings) [branch: wip-MDL-28954-m29 | CI Job ] phplint (0/0) , php (1/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , master (2 errors / 2 warnings) [branch: wip-MDL-28954-master | CI Job ] phplint (0/0) , php (1/2) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
              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 -

              This seems like an improvement rather than a bugfix to me? I don't imagine this was ever supported.

              Show
              poltawski Dan Poltawski added a comment - This seems like an improvement rather than a bugfix to me? I don't imagine this was ever supported.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              I agree with Dan, yesterday while looking to the linked issue I came here and thought the same. If, till now it has been impossible to add images to cohort descriptions I'd consider this a new feature/improvement.

              Also, when I looked to code I had a thought about why the editor does show/allow to click the images/media button if the instance has been explicitly configured to avoid embed files. Maybe we should be dimming/hiding the button?

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - I agree with Dan, yesterday while looking to the linked issue I came here and thought the same. If, till now it has been impossible to add images to cohort descriptions I'd consider this a new feature/improvement. Also, when I looked to code I had a thought about why the editor does show/allow to click the images/media button if the instance has been explicitly configured to avoid embed files. Maybe we should be dimming/hiding the button? Ciao
              Hide
              marina Marina Glancy added a comment - - edited

              I'll leave it up to integrator to decide whether it's a bug or improvement.

              As for the images/media button - you are still able to insert an image/media/link specifying URL

              Show
              marina Marina Glancy added a comment - - edited I'll leave it up to integrator to decide whether it's a bug or improvement. As for the images/media button - you are still able to insert an image/media/link specifying URL
              Hide
              poltawski Dan Poltawski added a comment -

              Stopping review of this and converting to an improvement - we'll follow the procedure - integrate this next week and can have a backport request if it wants to go to stables.

              Show
              poltawski Dan Poltawski added a comment - Stopping review of this and converting to an improvement - we'll follow the procedure - integrate this next week and can have a backport request if it wants to go to stables.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Now that the on-sync period has ended, this issue is being un-held and will be processed normally as part of the integration queue. Thanks for your patience.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Now that the on-sync period has ended, this issue is being un-held and will be processed normally as part of the integration queue. Thanks for your patience.
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-28954 using repository: https://github.com/marinaglancy/moodle

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-28954 using repository: https://github.com/marinaglancy/moodle MOODLE_28_STABLE (1 errors / 0 warnings) [branch: wip-MDL-28954-m28 | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_29_STABLE (0 errors / 0 warnings) [branch: wip-MDL-28954-m29 | CI Job ] master (1 errors / 0 warnings) [branch: wip-MDL-28954-master | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-28954 using repository: https://github.com/marinaglancy/moodle

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-28954 using repository: https://github.com/marinaglancy/moodle master (0 errors / 0 warnings) [branch: wip-MDL-28954-master | CI Job ] More information about this report
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (master only), thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for fixing this Marina,

              Works as expected, images can be changed/added and retained when changing context. Also, user with cohort/view can only view images, rest get "filenotfound" error.

              Passing...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Marina, Works as expected, images can be changed/added and retained when changing context. Also, user with cohort/view can only view images, rest get "filenotfound" error. Passing...
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for your contributions and hard work! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

              “Anyone who has never made a mistake has never tried anything new.”
              – Albert Einstein

              Show
              poltawski Dan Poltawski added a comment - Thanks for your contributions and hard work! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. “Anyone who has never made a mistake has never tried anything new.” – Albert Einstein
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              MDLQA-7158 has been modified to ensure this change is tested.

              Show
              rajeshtaneja Rajesh Taneja added a comment - MDLQA-7158 has been modified to ensure this change is tested.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    16/Nov/15