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

          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

              People

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

                Dates

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