Moodle
  1. Moodle
  2. MDL-27896

Incorrect context set in course/edit.php prevent emoticons to observe filters status properly

    Details

    • Testing Instructions:
      Hide
                            1. Test editor filters ############
                              1. Log in as Admin
                              2. Turn emoticons on (Site administration -> Plugins -> filters -> Manage filters -> Display emoticons as images)
                              3. Turn editing on
                              4. Select a course
                              5. Click settings -> Edit settings
                              6. Click on emoticon icon and add an emoticon in course summary
                              7. Save course
                              8. Turn emoticons off for the course (Settings -> Filters -> Display emoticons as images )
                              9. Click settings -> Edit settings
                              10. Emoticon icon should not be visible, but text version of emoticon should be shown.
                              11. Select another course and click settings -> edit settings
                              12. You should be able to see emoticons button in course summary editor.
                              13. Try perform similar test for cohort

      test category:
      1. Create a category
      2. turn off emoticon filter
      3. Add a subcategory and emoticon button should not be visible
      4. Add course and emoticons should not be visible
      5. Turn emoticon "on" on category
      6. repeat 3 and 4, and emoticon button should be visible.

                            1. Test files ############
                              1. edit category and insert image
                              2. Save category and edit again, image should be visible
                              3. Try create multiple categories and sub-categories and repeat step 1,2.
                              4. Try test the same in cohort, course, assignment, feedback, lesson, wiki, workshop and user
      Show
      Test editor filters ############ 1. Log in as Admin 2. Turn emoticons on (Site administration -> Plugins -> filters -> Manage filters -> Display emoticons as images) 3. Turn editing on 4. Select a course 5. Click settings -> Edit settings 6. Click on emoticon icon and add an emoticon in course summary 7. Save course 8. Turn emoticons off for the course (Settings -> Filters -> Display emoticons as images ) 9. Click settings -> Edit settings 10. Emoticon icon should not be visible, but text version of emoticon should be shown. 11. Select another course and click settings -> edit settings 12. You should be able to see emoticons button in course summary editor. 13. Try perform similar test for cohort test category: 1. Create a category 2. turn off emoticon filter 3. Add a subcategory and emoticon button should not be visible 4. Add course and emoticons should not be visible 5. Turn emoticon "on" on category 6. repeat 3 and 4, and emoticon button should be visible. Test files ############ 1. edit category and insert image 2. Save category and edit again, image should be visible 3. Try create multiple categories and sub-categories and repeat step 1,2. 4. Try test the same in cohort, course, assignment, feedback, lesson, wiki, workshop and user
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-27896
    • Rank:
      17547

      Description

      While integrating MDL-27500 and playing with new code was detected that the course->summary editor was not observing the filters status for the course context, causing the emoticons button (tinymce plugin) to be shown / hidden incorrectly.

      After a nice trace session with David, it seems that we need to:

      1) Add the missing 'context' element to the editoroptions array in course/edit.php

      And also we have agreed it would be great to have some central check in order to find anomalous situations like that, so we have decided to:

      2) Modify file_prepare_editor() (not used in all forms but in a hight %, and it's the only central point where both $options and $context are available) to:

      2a) missing both OK
      2b) $context !== $options['context'] then EXCEPTION
      2c) missing one of them DEBUGGING_DEVELOPER

      1. chat.txt
        13 kB
        David Mudrak

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Hello Eloy,

          While creating editor (using file_prepare_standard_editor), currently we pass context and editor options.

          can't we do something like:
          1. Give preference to $option['context']
          2. If not available and $context != null, then $option['context'] = $context(so we don't lose context)
          3. If missing both then no problem.
          4. If both are present and different then DEBUGGING_DEVELOPER

          Please advice
          Cheers
          Rajesh

          Show
          Rajesh Taneja added a comment - Hello Eloy, While creating editor (using file_prepare_standard_editor), currently we pass context and editor options. can't we do something like: 1. Give preference to $option ['context'] 2. If not available and $context != null, then $option ['context'] = $context(so we don't lose context) 3. If missing both then no problem. 4. If both are present and different then DEBUGGING_DEVELOPER Please advice Cheers Rajesh
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Adding David here, because I remember we looked to a lot of combinations / possibilities and, at the end we finished with the proposed here being the best (BC, proactively warning devs, exception on clearly wrong situations...).

          Tomorrow I'll refresh my mind with this, but surely David will fix my bad memory-storage :-P

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Adding David here, because I remember we looked to a lot of combinations / possibilities and, at the end we finished with the proposed here being the best (BC, proactively warning devs, exception on clearly wrong situations...). Tomorrow I'll refresh my mind with this, but surely David will fix my bad memory-storage :-P
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy

          Show
          Rajesh Taneja added a comment - Thanks Eloy
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rajesh,

          it sounds to me that we explored the idea of using only one of them (give preference to it, your 1. above) but at the end it was not clear why one should get preference not why code should "fix" anomalous situations at all. If we follow the "give preference" approach, wrong caller code will continue working without ever noticing it.

          I think that's the ultimate cause for deciding about our radical 2b and 2c above. If the context differs it's clearly one exception, no way that code can have 2 different contexts at the same time. Needs fixing on callers for sure. And if one of them is missing it means that "something" (the smilies, or the files... or whatever) won't work ok in the editor, so we debug info about that.

          I think that was more or less the rationale against our 2a...2c proposal above. Unluckly, I've tried to access to my chat history with David but it's soooo big that causes my jabber client to die, grrr. Perhaps David can confirm/revoke some of my comments above.

          Of course, this will nee careful integration + testing, something like:

          a) implement the agreed patch, surely master only.
          b) run a BIG test over ALL forms in Moodle, looking for exceptions & debugging, annotate them
          c) fix the "wrong forms" in master, but backport changes to those forms in 21_STABLE when possible.
          d) optionally, backport the a) patch to 21_STABLE

          So at the end, based on a) and b) we'll detect the offending editors and will fix them both in master and 21_STABLE, but without including the "radical" patch there. Only when all the forms are ok we could consider backporting the a) patch from master to 21_STABLE (although I don't think we'll do so).

          And that's all, feel free to share/comment the problem there. Perhpas they are only 2 forms, who knows! I bet they will be, at least, 3, lol!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rajesh, it sounds to me that we explored the idea of using only one of them (give preference to it, your 1. above) but at the end it was not clear why one should get preference not why code should "fix" anomalous situations at all. If we follow the "give preference" approach, wrong caller code will continue working without ever noticing it. I think that's the ultimate cause for deciding about our radical 2b and 2c above. If the context differs it's clearly one exception, no way that code can have 2 different contexts at the same time. Needs fixing on callers for sure. And if one of them is missing it means that "something" (the smilies, or the files... or whatever) won't work ok in the editor, so we debug info about that. I think that was more or less the rationale against our 2a...2c proposal above. Unluckly, I've tried to access to my chat history with David but it's soooo big that causes my jabber client to die, grrr. Perhaps David can confirm/revoke some of my comments above. Of course, this will nee careful integration + testing, something like: a) implement the agreed patch, surely master only. b) run a BIG test over ALL forms in Moodle, looking for exceptions & debugging, annotate them c) fix the "wrong forms" in master, but backport changes to those forms in 21_STABLE when possible. d) optionally, backport the a) patch to 21_STABLE So at the end, based on a) and b) we'll detect the offending editors and will fix them both in master and 21_STABLE, but without including the "radical" patch there. Only when all the forms are ok we could consider backporting the a) patch from master to 21_STABLE (although I don't think we'll do so). And that's all, feel free to share/comment the problem there. Perhpas they are only 2 forms, who knows! I bet they will be, at least, 3, lol! Ciao
          Hide
          Rajesh Taneja added a comment -

          Hi! Eloy,

          I agree that there can't be two context and that's what we should warn the developer about.

          At the same time it seems like, having approach 1, 2 (a, b) is like a band-aid to the broken functionality.
          As, we are passing $context to the function, the function should not expect $option['context'] at first place itself and should override it in all cases and pass it as $option['context'] to use_editor() (which expects context to be in $option['context']).

          As a counter measure, we can throw error if any caller function try to pass context via $option['context'] to detect wrong usage of it and later can be removed easily (As, this is something which file_prepare_standard_editor() should not expect at all.)

          My main concern for all this is why to pass duplicate parameters and confuse ourself of which is right and which is wrong (Which is happening now.).

          Please correct me if I am missing any point here.

          Thanks again for all the help.

          Regards
          Rajesh

          Show
          Rajesh Taneja added a comment - Hi! Eloy, I agree that there can't be two context and that's what we should warn the developer about. At the same time it seems like, having approach 1, 2 (a, b) is like a band-aid to the broken functionality. As, we are passing $context to the function, the function should not expect $option ['context'] at first place itself and should override it in all cases and pass it as $option ['context'] to use_editor() (which expects context to be in $option ['context'] ). As a counter measure, we can throw error if any caller function try to pass context via $option ['context'] to detect wrong usage of it and later can be removed easily (As, this is something which file_prepare_standard_editor() should not expect at all.) My main concern for all this is why to pass duplicate parameters and confuse ourself of which is right and which is wrong (Which is happening now.). Please correct me if I am missing any point here. Thanks again for all the help. Regards Rajesh
          Hide
          David Mudrak added a comment -

          Hi Rajesh. Please see the chat record attached. It is where this bug was spotted and analysed. We concluded first that a single place is the best way, too. But then we realized that the $editoroptions is used later again. And because we can't change APIs on stable branches (so we can't drop the $context parameter from the function), we must live with both.

          It's not nice at all, I know.

          Show
          David Mudrak added a comment - Hi Rajesh. Please see the chat record attached. It is where this bug was spotted and analysed. We concluded first that a single place is the best way, too. But then we realized that the $editoroptions is used later again. And because we can't change APIs on stable branches (so we can't drop the $context parameter from the function), we must live with both. It's not nice at all, I know.
          Hide
          Rajesh Taneja added a comment -

          Thanks David,

          That helped .
          Can we consider this in other way:
          1. file_prepare_standard_editor() is a general function and should get $context as one of the parameters (which it is taking now). Putting context in editor_option might make developer miss context (which happened in courses.) - In short leave it as it is.
          2. Now $option['context'] being an optional parameter, should be removed from all calls as this is a replication (And is confusing in long term)
          3. Add warning if $option['context'] is passed and leave that for few versions till we are sure that file_prepare_standard_editor() is used in correct way.
          4. Assign $option['context'] = $context, making sure we pass context to editor whenever required.

          This way we can safely get rid of wrong code usage, without breaking it and keep it simple so that no newbie (Like me) have doubts about what to pass and how.

          Please advice, which solution would you like me to go for

          Thanks
          -Rajesh

          Show
          Rajesh Taneja added a comment - Thanks David, That helped . Can we consider this in other way: 1. file_prepare_standard_editor() is a general function and should get $context as one of the parameters (which it is taking now). Putting context in editor_option might make developer miss context (which happened in courses.) - In short leave it as it is. 2. Now $option ['context'] being an optional parameter, should be removed from all calls as this is a replication (And is confusing in long term) 3. Add warning if $option ['context'] is passed and leave that for few versions till we are sure that file_prepare_standard_editor() is used in correct way. 4. Assign $option ['context'] = $context, making sure we pass context to editor whenever required. This way we can safely get rid of wrong code usage, without breaking it and keep it simple so that no newbie (Like me) have doubts about what to pass and how. Please advice, which solution would you like me to go for Thanks -Rajesh
          Hide
          Rossiani Wijaya added a comment -

          Hi Raj,

          This looks great.

          Show
          Rossiani Wijaya added a comment - Hi Raj, This looks great.
          Hide
          Petr Škoda added a comment -

          Hello,
          I like the general idea. There is one small problem, the "invalid_parameter_exception" is used for invalid parameters of web service or externallib functions. It means somebody in non-core code submitted invalid parameter. IN this place I think it would be better to use standard "coding_exception"

          Thanks for the patch anyway.

          Petr

          Show
          Petr Škoda added a comment - Hello, I like the general idea. There is one small problem, the "invalid_parameter_exception" is used for invalid parameters of web service or externallib functions. It means somebody in non-core code submitted invalid parameter. IN this place I think it would be better to use standard "coding_exception" Thanks for the patch anyway. Petr
          Hide
          David Mudrak added a comment -

          Raj, thanks for the patch. I agree with Petr - we should throw coding_exception here.

          Comparing context objects may become tricky in the future when we eventually replace stdClass with a context class. Maybe we should compare $context->id here.

          Also, IIRC the discussion with Mr. Lafuente, that silent assignment to $options['context'] = $context; may be tricky because the caller, who passed the $option, could use it later and the context is not there. Are we aware of that?

          Raj, can you please grep through the code base for the places where the file_prepare_standard_editor() and friends are used and make sure that the context are passed correctly there? If not, please apply the same fix as in course/edit.php.

          Show
          David Mudrak added a comment - Raj, thanks for the patch. I agree with Petr - we should throw coding_exception here. Comparing context objects may become tricky in the future when we eventually replace stdClass with a context class. Maybe we should compare $context->id here. Also, IIRC the discussion with Mr. Lafuente, that silent assignment to $options ['context'] = $context; may be tricky because the caller, who passed the $option, could use it later and the context is not there. Are we aware of that? Raj, can you please grep through the code base for the places where the file_prepare_standard_editor() and friends are used and make sure that the context are passed correctly there? If not, please apply the same fix as in course/edit.php.
          Hide
          Petr Škoda added a comment -

          thanks david!

          Show
          Petr Škoda added a comment - thanks david!
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr and David,

          I will fix this.
          @David, When I greped for file_prepare_standard_editor(), I found the usage has been very inconsistent.

          Probably, I can make consistant calls to this api(as part of this fix).
          Please advice if this has to be a different Bug or it's fine to have it all fixed in this.

          Show
          Rajesh Taneja added a comment - Thanks Petr and David, I will fix this. @David, When I greped for file_prepare_standard_editor(), I found the usage has been very inconsistent. Probably, I can make consistant calls to this api(as part of this fix). Please advice if this has to be a different Bug or it's fine to have it all fixed in this.
          Hide
          David Mudrak added a comment -

          Thanks Raj. I would focus on this issue only in the patch. However, the idea was just to try and detect similar places that set the context in one place only, as the course/edit.php does. If you realize it's difficult, never mind. We can go and fix them as the debugging will appear.

          Show
          David Mudrak added a comment - Thanks Raj. I would focus on this issue only in the patch. However, the idea was just to try and detect similar places that set the context in one place only, as the course/edit.php does. If you realize it's difficult, never mind. We can go and fix them as the debugging will appear.
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the help David

          I have integrated following inputs:

          1. replaced invalid_parameter_exception with coding_exception
          2. Now comparing $context->id and not objects.
          3. Removed silent assignment to $option['context'], (Although this was not silent - as we were alerting developer, But I agree there might be some use-case exceptions, so forcing this behavior is not Good)
          4. Fixed all use-cases for file_prepare_standard_editor.

          Can you please peer review the changes, before I send them for integration review.

          Cheers
          Rajesh

          Show
          Rajesh Taneja added a comment - Thanks for all the help David I have integrated following inputs: replaced invalid_parameter_exception with coding_exception Now comparing $context->id and not objects. Removed silent assignment to $option ['context'] , (Although this was not silent - as we were alerting developer, But I agree there might be some use-case exceptions, so forcing this behavior is not Good) Fixed all use-cases for file_prepare_standard_editor. Can you please peer review the changes, before I send them for integration review. Cheers Rajesh
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the inputs David

          Show
          Rajesh Taneja added a comment - Thanks for all the inputs David
          Hide
          Petr Škoda added a comment -

          If you are creating new category, course, activity or a block the context does not exist yet, but the filepicker/tinymce might want to know at lest the parent context. I suppose it would be ok to pass the parent context in $options['context'] and null in $context. I am not sure, maybe we should pass it as a separate parameter.

          Please discuss this issue with Dongsheng, I created the $context part, he is responsible for the filepicker internals.

          Show
          Petr Škoda added a comment - If you are creating new category, course, activity or a block the context does not exist yet, but the filepicker/tinymce might want to know at lest the parent context. I suppose it would be ok to pass the parent context in $options ['context'] and null in $context. I am not sure, maybe we should pass it as a separate parameter. Please discuss this issue with Dongsheng, I created the $context part, he is responsible for the filepicker internals.
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing this out Petr

          I had a chat with Sam and Dongsheng, and there seems to be a potential problem with my initial solution.
          I need some time to trace back this, as it hits couple of modules and need to be looked closely

          Show
          Rajesh Taneja added a comment - Thanks for pointing this out Petr I had a chat with Sam and Dongsheng, and there seems to be a potential problem with my initial solution. I need some time to trace back this, as it hits couple of modules and need to be looked closely
          Hide
          Rajesh Taneja added a comment -

          Modified the following:

          1. Added relevant context to all the modules. If current context is null then parent context is used (default is system context).
          2. Check is added in file_prepare_standard_editor to grab wrong context usage.
          Show
          Rajesh Taneja added a comment - Modified the following: Added relevant context to all the modules. If current context is null then parent context is used (default is system context). Check is added in file_prepare_standard_editor to grab wrong context usage.
          Hide
          Dongsheng Cai added a comment -

          Sorry wrong click

          Show
          Dongsheng Cai added a comment - Sorry wrong click
          Hide
          Dongsheng Cai added a comment -

          Looks good to me, thanks for fixing the context chaos

          Regards,
          Dongsheng Cai

          Show
          Dongsheng Cai added a comment - Looks good to me, thanks for fixing the context chaos Regards, Dongsheng Cai
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I'm reopening this issue this week sorry as I really think this needs to be closely looked at and properly reviewed and presently I am out of time.

          Looking at the changes it appears we are becoming more strict on passing contexts around and I see many place have already been upgraded by this patch to ensure that context has been set, however other areas directly creating editors or directly using the lower level API's have not been looked at, should they?

          Another thing I have noticed having only spent a brief time looking at code is in cohort/edit.php context is now being passed as an arg to file_prepare_standard editor and is either the system or a course category context. However in the form itself you can change the level the cohort is being created for and essentially switch the context the editor should be using.

          Lastly we need to consider whether we backport this or not.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I'm reopening this issue this week sorry as I really think this needs to be closely looked at and properly reviewed and presently I am out of time. Looking at the changes it appears we are becoming more strict on passing contexts around and I see many place have already been upgraded by this patch to ensure that context has been set, however other areas directly creating editors or directly using the lower level API's have not been looked at, should they? Another thing I have noticed having only spent a brief time looking at code is in cohort/edit.php context is now being passed as an arg to file_prepare_standard editor and is either the system or a course category context. However in the form itself you can change the level the cohort is being created for and essentially switch the context the editor should be using. Lastly we need to consider whether we backport this or not. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Submitting back for integration

          Show
          Rajesh Taneja added a comment - Submitting back for integration
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho,

          I think the patch is looking perfect, sweet.

          About the backporting concerns, really we are only throwing errors if the 2 contexts are defined and different (that is, indeed, one real bug). In the rest of cases, we are, simply, debugging if the editoroptions context is missing and the files context is defined. No changes in any other situation, so impact for users should be minimal and limited to real bugs. So, based on that, my +1 for this to be spread along all supported branches.

          More yet, I'd be happy if we get stricter along the time (2.2, 2.3...) and detect more and more situations or, why not, analyze if we can end suppressing the $context and use only the editor one or so. Really having 2 is far from optimal. But that's another story.

          Back to the current patch, there are some places where I'm not sure if we are doing ok or no. And those are the places where the context is used in a variable way (course/edit, course/editcategory, user/editadvanced, cohort/edit) or can be "moved" by the form (i.e. the UI allows to move the entity to another course category...). So I would suggest to:

          1) Add explicit testing instructions for those variable/movable places. With new entities (course, user, cohort..), edited ones and moved ones.
          2) user/editadvanced.php is using one $coursecontext for new users, not sure why.
          3) course/edit.php, on new course creation is passing the category context to the editor. Check it does not affect how/where files are handled/stored.

          So, +1 for one more iteration, but I think we are 99% in the way, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, I think the patch is looking perfect, sweet. About the backporting concerns, really we are only throwing errors if the 2 contexts are defined and different (that is, indeed, one real bug). In the rest of cases, we are, simply, debugging if the editoroptions context is missing and the files context is defined. No changes in any other situation, so impact for users should be minimal and limited to real bugs. So, based on that, my +1 for this to be spread along all supported branches. More yet, I'd be happy if we get stricter along the time (2.2, 2.3...) and detect more and more situations or, why not, analyze if we can end suppressing the $context and use only the editor one or so. Really having 2 is far from optimal. But that's another story. Back to the current patch, there are some places where I'm not sure if we are doing ok or no. And those are the places where the context is used in a variable way (course/edit, course/editcategory, user/editadvanced, cohort/edit) or can be "moved" by the form (i.e. the UI allows to move the entity to another course category...). So I would suggest to: 1) Add explicit testing instructions for those variable/movable places. With new entities (course, user, cohort..), edited ones and moved ones. 2) user/editadvanced.php is using one $coursecontext for new users, not sure why. 3) course/edit.php, on new course creation is passing the category context to the editor. Check it does not affect how/where files are handled/stored. So, +1 for one more iteration, but I think we are 99% in the way, thanks!
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Eloy

          • I have updated test instructions and tested most of the movable/variable places and it works as expected.
          • In user/editadvanced.php $coursecontext is used because it was set on top.
            if ($course->id == SITEID) {
                $coursecontext = get_context_instance(CONTEXT_SYSTEM); // SYSTEM context
            } else {
                $coursecontext = get_context_instance(CONTEXT_COURSE, $course->id); // Course context
            }
            
          • tested files and there seems to be few things which I am still looking at. We are passing itemid as 0 in file_prepare_standard_editor (course/editcategory.php). As such passing $context to file_prepare_standard_editor is not going to effect files. They are just draft files and finally when they get copied we have the exact context where the file needs to be moved (file_postupdate_standard_editor). Hence all seems to be fine in file area...

          Still testing further to make sure all works as expected.

          Thanks Sam for all your support

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Eloy I have updated test instructions and tested most of the movable/variable places and it works as expected. In user/editadvanced.php $coursecontext is used because it was set on top. if ($course->id == SITEID) { $coursecontext = get_context_instance(CONTEXT_SYSTEM); // SYSTEM context } else { $coursecontext = get_context_instance(CONTEXT_COURSE, $course->id); // Course context } tested files and there seems to be few things which I am still looking at. We are passing itemid as 0 in file_prepare_standard_editor (course/editcategory.php). As such passing $context to file_prepare_standard_editor is not going to effect files. They are just draft files and finally when they get copied we have the exact context where the file needs to be moved (file_postupdate_standard_editor). Hence all seems to be fine in file area... Still testing further to make sure all works as expected. Thanks Sam for all your support
          Hide
          Eloy Lafuente (stronk7) added a comment -

          thanks Rajesh!

          Show
          Eloy Lafuente (stronk7) added a comment - thanks Rajesh!
          Hide
          Rajesh Taneja added a comment - - edited

          Some more findings:
          Passing context in cohort is not a problem, because itemid = null and if either $context or $item == null then both are explicitly set to null in file_prepare_draft area.

          editcategory.php
          Is it acceptable to pass 0 as itemid in file_prepare_standard_editor?
          With my changes we were respecting parent category context for new categories, which tend to load parent files in file draft area. So the last check-in actually respect parent context making sure the parent files are not moved to draft area.

          Rest of the changes are safe as they just add context for editor and have no effect on draft area files/contents.

          I presume it's now safe to integrate this patch

          Show
          Rajesh Taneja added a comment - - edited Some more findings: Passing context in cohort is not a problem, because itemid = null and if either $context or $item == null then both are explicitly set to null in file_prepare_draft area. editcategory.php Is it acceptable to pass 0 as itemid in file_prepare_standard_editor? With my changes we were respecting parent category context for new categories, which tend to load parent files in file draft area. So the last check-in actually respect parent context making sure the parent files are not moved to draft area. Rest of the changes are safe as they just add context for editor and have no effect on draft area files/contents. I presume it's now safe to integrate this patch
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated, with one little extra commit fixing one whitespace occurrence.

          Note to testers, please test it into STABLE branch!

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated, with one little extra commit fixing one whitespace occurrence. Note to testers, please test it into STABLE branch! Thanks!
          Hide
          Michael de Raadt added a comment -

          Tested: I was able to add emoticons everywhere and control them with filter settings, including sub-categories where the parent category had a different setting.

          I was able to add images to descriptions all over the place, except to cohorts. I've launched this as a separate bug.

          Show
          Michael de Raadt added a comment - Tested: I was able to add emoticons everywhere and control them with filter settings, including sub-categories where the parent category had a different setting. I was able to add images to descriptions all over the place, except to cohorts. I've launched this as a separate bug.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks!

          Closing. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks! Closing. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: