Moodle
  1. Moodle
  2. MDL-30376

Glossary RSS feed generates error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.0.7, 2.1.4
    • Component/s: Glossary, RSS
    • Labels:
    • Environment:
      qa.moodle.net
    • Testing Instructions:
      Hide

      From QA test:
      1. Log in as a student and browse to a blog.
      2. Check that an orange RSS button is displayed on the page and that it links to the RSS feed for the blog.
      3. Browse to a database activity and check that an orange RSS button is displayed in the settings block and that it links to the RSS feed for the activity.
      4. Browse to a forum and check that an orange RSS button is displayed in the settings block and that it links to the RSS feed for the activity.
      5. Browse to a glossary and check that an orange RSS button is displayed in the settings block and that it links to the RSS feed for the activity.
      6. Log out then log in as a guest and repeat all the above steps.

      Show
      From QA test: 1. Log in as a student and browse to a blog. 2. Check that an orange RSS button is displayed on the page and that it links to the RSS feed for the blog. 3. Browse to a database activity and check that an orange RSS button is displayed in the settings block and that it links to the RSS feed for the activity. 4. Browse to a forum and check that an orange RSS button is displayed in the settings block and that it links to the RSS feed for the activity. 5. Browse to a glossary and check that an orange RSS button is displayed in the settings block and that it links to the RSS feed for the activity. 6. Log out then log in as a guest and repeat all the above steps.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30376-master
    • Rank:
      32985

      Description

      When requesting an RSS feed from a Glossary activity, an exception is displayed.

        Issue Links

          Activity

          Hide
          Nicolas Martignoni added a comment -

          More details about my testings.

          Step 2: there is no RSS icon when displaying a blog (as a student or a teacher). Tested with several themes.

          Step 6 (logged in as a guest)

          • Clicking on the RSS icon for a Glossary RSS displays an "RSS error"
          • There's no RSS icon for Forum RSS
          • For the Database activity, the RSS icon is displayed and works correctly
          Show
          Nicolas Martignoni added a comment - More details about my testings. Step 2: there is no RSS icon when displaying a blog (as a student or a teacher). Tested with several themes. Step 6 (logged in as a guest) Clicking on the RSS icon for a Glossary RSS displays an "RSS error" There's no RSS icon for Forum RSS For the Database activity, the RSS icon is displayed and works correctly
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Nicolas,

          can I suggest to split this into 3:

          • The Glossary RSS error (this).
          • The missing RSS icon for forum guests.
          • The missing RSS for blogs.

          They 3 are really separated components and I guess each one will need to follow its route. All them blocking MDLQA-1413, but separated.

          Thanks for your continuous collaboration! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Nicolas, can I suggest to split this into 3: The Glossary RSS error (this). The missing RSS icon for forum guests. The missing RSS for blogs. They 3 are really separated components and I guess each one will need to follow its route. All them blocking MDLQA-1413 , but separated. Thanks for your continuous collaboration! Ciao
          Hide
          Nicolas Martignoni added a comment -

          Ok Eloy: I created the 2 issues MDL-30385 and MDL-30386 for the others problems.

          Ciao

          Show
          Nicolas Martignoni added a comment - Ok Eloy: I created the 2 issues MDL-30385 and MDL-30386 for the others problems. Ciao
          Hide
          Charles Fulton added a comment -

          MDL-24870 introduced an is_enrolled() test for glossary RSS feeds which won't work for guest (nor administrators). I wrote a quick patch to restore guest access: https://github.com/mackensen/moodle/compare/master...MDL-30376.

          I think a longer term fix would be a specific read capability for glossary entries (similar to mod/data).

          Show
          Charles Fulton added a comment - MDL-24870 introduced an is_enrolled() test for glossary RSS feeds which won't work for guest (nor administrators). I wrote a quick patch to restore guest access: https://github.com/mackensen/moodle/compare/master...MDL-30376 . I think a longer term fix would be a specific read capability for glossary entries (similar to mod/data).
          Hide
          Jason Fowler added a comment -

          I have tried to reproduce this, but get a clean, clear RSS feed when I tried.

          Show
          Jason Fowler added a comment - I have tried to reproduce this, but get a clean, clear RSS feed when I tried.
          Hide
          Charles Fulton added a comment -

          Jason: you ought to get errors both as a non-enrolled administrator and as a guest. Instead of glossary items in the feed you should get a single error (as a feed item).

          Show
          Charles Fulton added a comment - Jason: you ought to get errors both as a non-enrolled administrator and as a guest. Instead of glossary items in the feed you should get a single error (as a feed item).
          Hide
          Jason Fowler added a comment -

          as a non-enrolled administrator, I get a good rss feed, with the following

          RSS Error
          28/11/11 13:11
          Error reading RSS data

          Show
          Jason Fowler added a comment - as a non-enrolled administrator, I get a good rss feed, with the following RSS Error 28/11/11 13:11 Error reading RSS data
          Hide
          Jason Fowler added a comment -

          And as a guest I get the following:

          <rss version="2.0">
          <channel>
          <title>Master Integration</title>
          <link>http://jason.moodle.local/imaster</link>
          <description/>
          <generator>Moodle</generator>
          <language>en</language>
          <copyright>© 2011 Master Integration</copyright>
          <image>
          <url>
          http://jason.moodle.local/imaster/theme/image.php?theme=standard&amp;image=i%2Frsssitelogo&amp;rev=344
          </url>
          <title>moodle</title>
          <link>http://jason.moodle.local/imaster</link>
          <width>140</width>
          <height>35</height>
          </image>
          <item>
          <title>RSS Error</title>
          <link>http://jason.moodle.local/imaster</link>
          <pubDate>Mon, 28 Nov 2011 06:06:12 GMT</pubDate>
          <description>Error reading RSS data</description>
          <guid isPermaLink="true">http://jason.moodle.local/imaster</guid>
          </item>
          </channel>
          </rss>

          Show
          Jason Fowler added a comment - And as a guest I get the following: <rss version="2.0"> <channel> <title>Master Integration</title> <link> http://jason.moodle.local/imaster </link> <description/> <generator>Moodle</generator> <language>en</language> <copyright>© 2011 Master Integration</copyright> <image> <url> http://jason.moodle.local/imaster/theme/image.php?theme=standard&amp;image=i%2Frsssitelogo&amp;rev=344 </url> <title>moodle</title> <link> http://jason.moodle.local/imaster </link> <width>140</width> <height>35</height> </image> <item> <title>RSS Error</title> <link> http://jason.moodle.local/imaster </link> <pubDate>Mon, 28 Nov 2011 06:06:12 GMT</pubDate> <description>Error reading RSS data</description> <guid isPermaLink="true"> http://jason.moodle.local/imaster </guid> </item> </channel> </rss>
          Hide
          Jason Fowler added a comment -

          This has been tested on a local install as well as the QA site, and the RSS feed shows up and works correctly once configured (globally and for the Glossary itself). For guest users and non-enrolled admins, you get the above RSS Feed, which hides the real details. I believe this is working the way it should really.

          Show
          Jason Fowler added a comment - This has been tested on a local install as well as the QA site, and the RSS feed shows up and works correctly once configured (globally and for the Glossary itself). For guest users and non-enrolled admins, you get the above RSS Feed, which hides the real details. I believe this is working the way it should really.
          Hide
          Jason Fowler added a comment -

          rss needs to be viewable as a guest

          Show
          Jason Fowler added a comment - rss needs to be viewable as a guest
          Hide
          Sam Hemelryk added a comment -

          Hi Jason,

          I've been looking at this now and I have to admit the introduction on the new capability is a big concern. Especially this close to release.
          I do agree that there should be a capability for viewing glossaries, however I think the capability solution still needs a lot more work. For one I think we would need to decide if you view the glossary (like mod/quiz:view.php) or whether you view glossary entries (like mod/forum:view discussion), and then once we have that all sorted we need to review all areas that display glossary content, things like the Random glossary entry block etc.

          I honestly think that we should avoid adding the capability at this point and leave that till after release.
          As for the solution at the moment I've just been having a look at the code and have been able to reproduce the problem by accessing a glossary RSS feed as an admin (ensure the admin is not enrolled in the course the glossary is within).
          The problem seems to boil down to the is_enrolled call in rsslib.php round line 25. Really that should be a call to can_access_course. Also worth noting even with the new capability that would be the case.
          These checks should reflect the checks made in glossary/lib.php which looked very good.

          Certainly at this point I think that we need to drop the idea of introducing a new capability and focus on the problem at hand.
          My +1 goes to solving this without the capability and then introducing the capability after release.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jason, I've been looking at this now and I have to admit the introduction on the new capability is a big concern. Especially this close to release. I do agree that there should be a capability for viewing glossaries, however I think the capability solution still needs a lot more work. For one I think we would need to decide if you view the glossary (like mod/quiz:view.php) or whether you view glossary entries (like mod/forum:view discussion), and then once we have that all sorted we need to review all areas that display glossary content, things like the Random glossary entry block etc. I honestly think that we should avoid adding the capability at this point and leave that till after release. As for the solution at the moment I've just been having a look at the code and have been able to reproduce the problem by accessing a glossary RSS feed as an admin (ensure the admin is not enrolled in the course the glossary is within). The problem seems to boil down to the is_enrolled call in rsslib.php round line 25. Really that should be a call to can_access_course. Also worth noting even with the new capability that would be the case. These checks should reflect the checks made in glossary/lib.php which looked very good. Certainly at this point I think that we need to drop the idea of introducing a new capability and focus on the problem at hand. My +1 goes to solving this without the capability and then introducing the capability after release. Cheers Sam
          Hide
          Jason Fowler added a comment -

          Changing approach to this issue as recommended by Sam and Michael, seems to work find now, without the capability

          Show
          Jason Fowler added a comment - Changing approach to this issue as recommended by Sam and Michael, seems to work find now, without the capability
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, Jason. It looks good now.

          I'm pushing this to integration so it can be integrated tonight.

          Show
          Michael de Raadt added a comment - Thanks for working on this, Jason. It looks good now. I'm pushing this to integration so it can be integrated tonight.
          Hide
          Sam Hemelryk added a comment -

          Thanks Jason, this has been integrated now.

          I backported the fix to the stable branches as well which required some touching up as function arguments had changed.

          Testers: Please test 20, 21, and master.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Jason, this has been integrated now. I backported the fix to the stable branches as well which required some touching up as function arguments had changed. Testers: Please test 20, 21, and master. Cheers Sam
          Hide
          Michael de Raadt added a comment - - edited

          Test result: passed

          I tested this in 2.0 stable, 2.1 stable and on qa.moodle.net.

          Show
          Michael de Raadt added a comment - - edited Test result: passed I tested this in 2.0 stable, 2.1 stable and on qa.moodle.net.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay!

          Closing and big thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay! Closing and big thanks!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: