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

Blog pages should be in site context always

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.5
    • Component/s: Blocks, Blog
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Test Requirements
      Enable blogs
      Test 1
      Add a block to the following pages:-

      1. Site blog listing page
      2. Site blog view page
      3. Site blog edit page
      1. Course blog listing page
      2. Course blog view page
      3. Course blog edit page
      1. User blog listing page
      2. User blog view page
      3. USer blog edit page
      1. Activity blog listing page
      2. Activity blog view page
      3. Activity blog edit page
      1. Blog Preferences page

      For each of the above visit around the site and make sure they are visible in the type of pages they were created in.(Everything now is in sitecontext, so block created in a listing page will be visible through out the site, same with other types)

      Test 2
      Edit all above blocks and make sure they all are created in system context
      Change the page type where they appear and browse around to make sure it is effective

      Test 3
      This needs a mandatory regression testing session with various blog related administration , as well as blog related permission. Please make sure 'moodle/blog:associatemodule' and 'moodle/blog:associatecourse' donot effect anything in blogs.

      Test 4
      Make sure you can access blog rss feeds whenever you can access the blogs

      Test 5
      In a fresh install make sure 'moodle/blog:associatemodule' and 'moodle/blog:associatecourse' is not given to any role . Also make sure those caps mention that they are deprecated in the "define role permissions"(Home ► Site administration ► Users ► Permissions ► Define roles>edit) page.

      Show
      Test Requirements Enable blogs Test 1 Add a block to the following pages:- Site blog listing page Site blog view page Site blog edit page Course blog listing page Course blog view page Course blog edit page User blog listing page User blog view page USer blog edit page Activity blog listing page Activity blog view page Activity blog edit page Blog Preferences page For each of the above visit around the site and make sure they are visible in the type of pages they were created in.(Everything now is in sitecontext, so block created in a listing page will be visible through out the site, same with other types) Test 2 Edit all above blocks and make sure they all are created in system context Change the page type where they appear and browse around to make sure it is effective Test 3 This needs a mandatory regression testing session with various blog related administration , as well as blog related permission. Please make sure 'moodle/blog:associatemodule' and 'moodle/blog:associatecourse' donot effect anything in blogs. Test 4 Make sure you can access blog rss feeds whenever you can access the blogs Test 5 In a fresh install make sure 'moodle/blog:associatemodule' and 'moodle/blog:associatecourse' is not given to any role . Also make sure those caps mention that they are deprecated in the "define role permissions"(Home ► Site administration ► Users ► Permissions ► Define roles>edit) page.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-27814-master

      Description

      Add a block to your blog (Navigation -> site pages -> blogs). Edit the block's settings and select "blog editing pages" from the page contexts drop down. hit save and where did the block go? I'm guessing it should now only appear on the edit blog post page. It doesnt seem to appear anywhere. Its just gone.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            ankit_frenz Ankit Agarwal added a comment -

            This is what is happening:-

            1. Try creating a block on the edit a blog entry page for a course
              Although this page is supposed to be in course context, it is in system context. Thus the new block created in this contest is visible site wide in all blog editing pages. which infact is another security issue (I think).
            2. Try creating a block on blog listing page for a course.
              Here the block is created in a user context (since a user id is passed on). This is the reason when we edit and set it to display in blog editing pages it vanishes. since those pages appear to be in system context.
            Show
            ankit_frenz Ankit Agarwal added a comment - This is what is happening:- Try creating a block on the edit a blog entry page for a course Although this page is supposed to be in course context, it is in system context. Thus the new block created in this contest is visible site wide in all blog editing pages. which infact is another security issue (I think). Try creating a block on blog listing page for a course. Here the block is created in a user context (since a user id is passed on). This is the reason when we edit and set it to display in blog editing pages it vanishes. since those pages appear to be in system context.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            As per the discussion https://moodle.org/local/chatlogs/index.php?conversationid=12346, all blog pages should be in system context.

            Show
            ankit_frenz Ankit Agarwal added a comment - As per the discussion https://moodle.org/local/chatlogs/index.php?conversationid=12346 , all blog pages should be in system context.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Requesting a review.

            Show
            ankit_frenz Ankit Agarwal added a comment - Requesting a review.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Patch looks good.

            The following line in blog/index.php could probably moved to approx. line 60. I will leave it up to you to make the change.
            $PAGE->set_context(context_system::instance());

            [y] Syntax
            [-] Output
            [y] Whitespace
            [-] Language
            [-] Databases
            [-] Testing
            [-] Security
            [-] Documentation
            [y] Git
            [y] Sanity check

            Show
            rwijaya Rossiani Wijaya added a comment - Patch looks good. The following line in blog/index.php could probably moved to approx. line 60. I will leave it up to you to make the change. $PAGE->set_context(context_system::instance()); [y] Syntax [-] Output [y] Whitespace [-] Language [-] Databases [-] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            The use of blog_get_context_url() doesn't make sense anymore, so I have deprecated it. It was redundant code, the same thing was already handled by blog_get_headers() based on passed params.

            Also adding Aparup as watcher, so he can make sure this function is not used a lot in plugins.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - The use of blog_get_context_url() doesn't make sense anymore, so I have deprecated it. It was redundant code, the same thing was already handled by blog_get_headers() based on passed params. Also adding Aparup as watcher, so he can make sure this function is not used a lot in plugins. Thanks
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for patch Ankit,

            As discussed, I am not sure if this is the approach we should take. This will break images, blocks present on blog as context will remain same (course/module/site), but page will popup with new context (site context with your patch).

            So if we plan to go this way then you might have to

            1. Fix context for related contents on blog.
            2. Give access to users to view course/activity blog even though they are not enrolled in course.
            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for patch Ankit, As discussed, I am not sure if this is the approach we should take. This will break images, blocks present on blog as context will remain same (course/module/site), but page will popup with new context (site context with your patch). So if we plan to go this way then you might have to Fix context for related contents on blog. Give access to users to view course/activity blog even though they are not enrolled in course.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            On discussion with Raj and Dan, and some deeper investigation following things were discovered:-

            1. Since all blogs are at system level, we should allow any user who has blogview permission at system level to read all blogs, irrespective of blog association. That is I should be able to view blog about a course by someone , even if I donot have access to the course. Even if we agree this is the expected behaviour (and I donot think everyone will agree), we will be changing an existing behaviour, effecting each and every moodle site which uses blogs. This also leads to the question should I be able to blog about that course?
            2. All images and attachments associated with existing blogs will need to be moved to site context, effectively allowing anyone access to it.
            3. Blocks added to any blog page would be visible site wide.
            4. Existing blocks on various blog page are a bit of concern. As we cannot simply migrate them to system context as they might have sensitive information. If we leave them at current context, they will be lost. We can't just delete theme either.
            Show
            ankit_frenz Ankit Agarwal added a comment - On discussion with Raj and Dan, and some deeper investigation following things were discovered:- Since all blogs are at system level, we should allow any user who has blogview permission at system level to read all blogs, irrespective of blog association. That is I should be able to view blog about a course by someone , even if I donot have access to the course. Even if we agree this is the expected behaviour (and I donot think everyone will agree), we will be changing an existing behaviour, effecting each and every moodle site which uses blogs. This also leads to the question should I be able to blog about that course? All images and attachments associated with existing blogs will need to be moved to site context, effectively allowing anyone access to it. Blocks added to any blog page would be visible site wide. Existing blocks on various blog page are a bit of concern. As we cannot simply migrate them to system context as they might have sensitive information. If we leave them at current context, they will be lost. We can't just delete theme either.
            Hide
            dougiamas Martin Dougiamas added a comment -

            1) On the internet, blogs are about talking publically about things. You never choose your readers. In the Moodle world it's the same concept, except the "world" is the institution. So yes all blog entries should be visible to someone looking at any blog in Moodle. The association is just that. It's like me blogging on dougiamas.com about something that happened at work.

            If someone wants a "course blog" then I tell them to start a forum, there is even a blog format for forums that makes them look more like blogs. There are also course blog activity addons.

            2) Should be IMO. You mean they weren't?

            3) Yep.

            4) Hmmmm

            Show
            dougiamas Martin Dougiamas added a comment - 1) On the internet, blogs are about talking publically about things. You never choose your readers. In the Moodle world it's the same concept, except the "world" is the institution. So yes all blog entries should be visible to someone looking at any blog in Moodle. The association is just that. It's like me blogging on dougiamas.com about something that happened at work. If someone wants a "course blog" then I tell them to start a forum, there is even a blog format for forums that makes them look more like blogs. There are also course blog activity addons. 2) Should be IMO. You mean they weren't? 3) Yep. 4) Hmmmm
            Hide
            dougiamas Martin Dougiamas added a comment -

            4) I guess the risk of just "losing them" is very low. This might only affect 2.2 and later. Blogs are not widely used anyway, and blocks on those pages less so. It is probably OK just to rectify the contexts and warn upgraders that some blocks may dissappear.

            Show
            dougiamas Martin Dougiamas added a comment - 4) I guess the risk of just "losing them" is very low. This might only affect 2.2 and later. Blogs are not widely used anyway, and blocks on those pages less so. It is probably OK just to rectify the contexts and warn upgraders that some blocks may dissappear.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            As discussed following changes are introduced by the patch.

            1. Every blog page is in site context now
            2. Old blocks on block pages are not touched
            3. 'moodle/blog:associatemodule' and 'moodle/blog:associatecourse' haven been deprecated based on the method discussed during scrum
            4. Now anyone can blog about anything as in normal blogs.(Even if they are not a part of the course/mod)
            5. Some minor code re-factoring where needed.
            6. Rss feeds for blogs are also in site context now.
            7. Provided patch only for master as this no way can be backported.
            8. The situations that blocks can be lost should be clearly explained in release docs.
            Show
            ankit_frenz Ankit Agarwal added a comment - As discussed following changes are introduced by the patch. Every blog page is in site context now Old blocks on block pages are not touched 'moodle/blog:associatemodule' and 'moodle/blog:associatecourse' haven been deprecated based on the method discussed during scrum Now anyone can blog about anything as in normal blogs.(Even if they are not a part of the course/mod) Some minor code re-factoring where needed. Rss feeds for blogs are also in site context now. Provided patch only for master as this no way can be backported. The situations that blocks can be lost should be clearly explained in release docs.
            Hide
            damyon Damyon Wiese added a comment -

            Hi Ankit,

            The changes look OK to me and reflect the change in context - except you have not addressed this point:

            "All images and attachments associated with existing blogs will need to be moved to site context, effectively allowing anyone access to it."

            You need an upgrade step to move the blog files from any other context to the system context.

            Show
            damyon Damyon Wiese added a comment - Hi Ankit, The changes look OK to me and reflect the change in context - except you have not addressed this point: "All images and attachments associated with existing blogs will need to be moved to site context, effectively allowing anyone access to it." You need an upgrade step to move the blog files from any other context to the system context.
            Hide
            damyon Damyon Wiese added a comment -

            I also haven't verified that the blog unit tests and behat tests still pass - if you can verify that is OK then with the upgrade step this is +1 from me.

            Show
            damyon Damyon Wiese added a comment - I also haven't verified that the blog unit tests and behat tests still pass - if you can verify that is OK then with the upgrade step this is +1 from me.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Damyon,
            Thanks for the review.

            "All images and attachments associated with existing blogs will need to be moved to site context, effectively allowing anyone access to it."

            I verified this, they are already in site context. The only images associated with blocks were in a different context, which we decided not to touch.

            $entry = file_prepare_standard_editor($entry, 'summary', $summaryoptions, $sitecontext, 'blog', 'post', $entry->id);
            $entry = file_prepare_standard_filemanager($entry, 'attachment', $attachmentoptions, $sitecontext, 'blog', 'attachment', $entry->id);

            Unit tests and behat tests are all passing.
            Pushing for integration
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Damyon, Thanks for the review. "All images and attachments associated with existing blogs will need to be moved to site context, effectively allowing anyone access to it." I verified this, they are already in site context. The only images associated with blocks were in a different context, which we decided not to touch. $entry = file_prepare_standard_editor($entry, 'summary', $summaryoptions, $sitecontext, 'blog', 'post', $entry->id); $entry = file_prepare_standard_filemanager($entry, 'attachment', $attachmentoptions, $sitecontext, 'blog', 'attachment', $entry->id); Unit tests and behat tests are all passing. Pushing for integration Thanks
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            ankit_frenz Ankit Agarwal added a comment -

            rebased
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - rebased Thanks
            Hide
            poltawski Dan Poltawski added a comment -

            Well done for getting through this tricky issue - i've integrated it to master..

            Please can you make sure to work with Helen/Mary to ensure that we have the implications of this properly documented in the release notes.

            Show
            poltawski Dan Poltawski added a comment - Well done for getting through this tricky issue - i've integrated it to master.. Please can you make sure to work with Helen/Mary to ensure that we have the implications of this properly documented in the release notes.
            Hide
            dmonllao David Monllaó added a comment -

            It passes, tested in master

            Show
            dmonllao David Monllaó added a comment - It passes, tested in master
            Hide
            poltawski Dan Poltawski added a comment -

            I'll press the pass button then

            Show
            poltawski Dan Poltawski added a comment - I'll press the pass button then
            Hide
            dmonllao David Monllaó added a comment -

            Ups, sorry I forgot

            Show
            dmonllao David Monllaó added a comment - Ups, sorry I forgot
            Hide
            poltawski Dan Poltawski added a comment -

            Blooming Marvelous! It's time for a knees up - your changes are upstream!

            Thanks for making Moodle better!

            Toodle pip

            Show
            poltawski Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13