Uploaded image for project: '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

      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

        Gliffy Diagrams

        1. chat.txt
          13 kB
          David Mudrak

          Issue Links

            Activity

            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            stronk7 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
            stronk7 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
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Eloy

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Eloy
            Hide
            stronk7 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
            stronk7 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
            rajeshtaneja 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
            rajeshtaneja 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
            mudrd8mz 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
            mudrd8mz 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
            rajeshtaneja 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
            rajeshtaneja 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
            rwijaya Rossiani Wijaya added a comment -

            Hi Raj,

            This looks great.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Raj, This looks great.
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            mudrd8mz 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
            mudrd8mz 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
            skodak Petr Skoda added a comment -

            thanks david!

            Show
            skodak Petr Skoda added a comment - thanks david!
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            mudrd8mz 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
            mudrd8mz 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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for all the inputs David

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for all the inputs David
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja 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
            rajeshtaneja 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 Dongsheng Cai added a comment -

            Sorry wrong click

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

            Looks good to me, thanks for fixing the context chaos

            Regards,
            Dongsheng Cai

            Show
            dongsheng Dongsheng Cai added a comment - Looks good to me, thanks for fixing the context chaos Regards, Dongsheng Cai
            Hide
            samhemelryk 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
            samhemelryk 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
            rajeshtaneja Rajesh Taneja added a comment -

            Submitting back for integration

            Show
            rajeshtaneja Rajesh Taneja added a comment - Submitting back for integration
            Hide
            stronk7 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
            stronk7 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
            rajeshtaneja 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
            rajeshtaneja 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            thanks Rajesh!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - thanks Rajesh!
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            stronk7 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
            stronk7 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
            salvetore 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
            salvetore 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  10/Oct/11