Moodle
  1. Moodle
  2. MDL-30482

Capability for viewing glossary entries

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      PLEASE PERFORM THESE STEPS IN ORDER

      1. Login as admin
      2. Enable RSS for the site and the glossaries in the site-wide prefernces
      3. Set an RSS value and number of entries in the glossary settings
      4. Create a glossary with some entries
      5. Turn on the auto-link glossary filter
      6. Create a page with some of the words from the glossary entries in it
      7. Add the Random glossary block
      8. Add the recent activity block
      9. Create a glossary activity on the front page
      10. Log out and check the front page is showing without errors
      11. Allow students read access to the glossary (System -> Users -> Permissions -> Define Roles) [This should already be on by default]
      12. Log in as a student enrolled in the course
      13. Check you can see the glossary in the recent activity block
      14. Check you can see the glossary entries in the recent activity block
      15. Check you can see the random glossary block
      16. Check you can view the glossary in the course
      17. Check you can view the entries in the glossary
      18. Check you can see the RSS feed for the glossary
      19. Check you can see the auto-linked items on the page you created
      20. In a different browser, Login as admin and deny the student access to read the glossary
      21. As a student enrolled in the course
      22. Check you can not see the glossary on the front page
      23. Check you can not see the glossary in the recent activity block
      24. Check you can not see the random glossary block
      25. Check you can not view the entries in the glossary
      26. Check you can not see the RSS feed for the glossary
      27. Check you can not see the auto-linked items on the page you created
      28. Repeat the steps 12-27 for guests as well
      Show
      PLEASE PERFORM THESE STEPS IN ORDER Login as admin Enable RSS for the site and the glossaries in the site-wide prefernces Set an RSS value and number of entries in the glossary settings Create a glossary with some entries Turn on the auto-link glossary filter Create a page with some of the words from the glossary entries in it Add the Random glossary block Add the recent activity block Create a glossary activity on the front page Log out and check the front page is showing without errors Allow students read access to the glossary (System -> Users -> Permissions -> Define Roles) [This should already be on by default] Log in as a student enrolled in the course Check you can see the glossary in the recent activity block Check you can see the glossary entries in the recent activity block Check you can see the random glossary block Check you can view the glossary in the course Check you can view the entries in the glossary Check you can see the RSS feed for the glossary Check you can see the auto-linked items on the page you created In a different browser, Login as admin and deny the student access to read the glossary As a student enrolled in the course Check you can not see the glossary on the front page Check you can not see the glossary in the recent activity block Check you can not see the random glossary block Check you can not view the entries in the glossary Check you can not see the RSS feed for the glossary Check you can not see the auto-linked items on the page you created Repeat the steps 12-27 for guests as well
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30482-master
    • Rank:
      33160

      Description

      This is a follow-up to MDLQA-1413 and MDL-30376. The lack of a specific 'read' capability for glossary entries complicates access control for RSS feeds. This patch adds a new capability for viewing glossary entries.

      Patch against current master: https://github.com/mackensen/moodle/compare/master...MDL-30482. The new capability is mod/glossary:read (following the convention of mod/glossary:write) and is based on mod/data:readentry.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting this and sharing a solution.

          Show
          Michael de Raadt added a comment - Thanks for suggesting this and sharing a solution.
          Hide
          Michael de Raadt added a comment -

          I'm promoting this issue and assigning it to Jason so he can continue the work he has started in MDL-30376.

          Show
          Michael de Raadt added a comment - I'm promoting this issue and assigning it to Jason so he can continue the work he has started in MDL-30376 .
          Hide
          Jason Fowler added a comment -

          Code should be a simple cherry pick for 2.2

          Show
          Jason Fowler added a comment - Code should be a simple cherry pick for 2.2
          Hide
          Ankit Agarwal added a comment -

          Hi Jason,
          Changes are looking good, but I noticed a small issue

                  if (!has_capability('mod/glossary:read', $context)) {
                      return false;
                  }
          
          • Lets say we have 3 glossaries in a course
          • Now we configure the glossaries so that students have read permission on two but not on the third
          • The above if condition will always return false so the user is not able to see anything in the "Recent activities block (Glossary part)" where as IMO he should be able to see the activities from the two Glossaries on which he has read permissions.

          Rest everything looks spot on to me.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jason, Changes are looking good, but I noticed a small issue if (!has_capability('mod/glossary:read', $context)) { return false ; } Lets say we have 3 glossaries in a course Now we configure the glossaries so that students have read permission on two but not on the third The above if condition will always return false so the user is not able to see anything in the "Recent activities block (Glossary part)" where as IMO he should be able to see the activities from the two Glossaries on which he has read permissions. Rest everything looks spot on to me. Thanks
          Hide
          Ankit Agarwal added a comment -

          Looks good to me!
          Thanks

          Show
          Ankit Agarwal added a comment - Looks good to me! Thanks
          Hide
          Aparup Banerjee added a comment -

          I'll assume from the one way conversation here that Jason has used his ninja skills to fix things up :-p

          Show
          Aparup Banerjee added a comment - I'll assume from the one way conversation here that Jason has used his ninja skills to fix things up :-p
          Hide
          Ankit Agarwal added a comment -

          And a Ninja never leaves any trace of his presence

          Show
          Ankit Agarwal added a comment - And a Ninja never leaves any trace of his presence
          Hide
          Aparup Banerjee added a comment -

          lol, we need some testing instructions here too :-p

          Show
          Aparup Banerjee added a comment - lol, we need some testing instructions here too :-p
          Hide
          Jason Fowler added a comment -

          testing instructions added

          Show
          Jason Fowler added a comment - testing instructions added
          Hide
          Aparup Banerjee added a comment -

          Thanks all, this has been integrated and is up for testing

          Show
          Aparup Banerjee added a comment - Thanks all, this has been integrated and is up for testing
          Hide
          Michael de Raadt added a comment -

          Sorry, I was hoping to test this this afternoon, but it doesn't look like I'm going to be to be able to. I'm keen to test this, but I'm going to stop testing and pick this up tomorrow if it is still available.

          Show
          Michael de Raadt added a comment - Sorry, I was hoping to test this this afternoon, but it doesn't look like I'm going to be to be able to. I'm keen to test this, but I'm going to stop testing and pick this up tomorrow if it is still available.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Side note: Don't forget to test some frontpage glossary! It may be failing there.
          Offtopic: "read" (horrible), "view" is way more beautiful. Too late anyway.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Side note: Don't forget to test some frontpage glossary! It may be failing there. Offtopic: "read" (horrible), "view" is way more beautiful. Too late anyway.
          Hide
          Michael de Raadt added a comment -

          Restarting this test.

          Show
          Michael de Raadt added a comment - Restarting this test.
          Hide
          Michael de Raadt added a comment -

          Test result: failed.

          It would be good to see some "how to" in a couple of steps there, such as step 8. At least you should direct the tester to where this change should be made.

          In the glossary settings, Autolinking needs to be turned on and the RSS settings have to change.

          When I turned off student permission to read glossary entries and returned to the course page I was presented with the following error. I was controlling the permission at the course level.

          Debug info: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY ge.timemodified ASC LIMIT 0, 51' at line 3
          SELECT ge.id, ge.concept, ge.approved, ge.timemodified, ge.glossaryid,
          u.id AS userid,u.picture,u.firstname,u.lastname,u.imagealt,u.email FROM mdl_glossary_entries ge 
          JOIN mdl_user u ON u.id = ge.userid WHERE ge.timemodified > ? AND () ORDER BY ge.timemodified ASC LIMIT 0, 51
          [array (
          0 => 1323664300,
          )]
          Stack trace:
          line 394 of \lib\dml\moodle_database.php: dml_read_exception thrown
          line 809 of \lib\dml\mysqli_native_moodle_database.php: call to moodle_database->query_end()
          line 380 of \mod\glossary\lib.php: call to mysqli_native_moodle_database->get_records_sql()
          line 1021 of \course\lib.php: call to glossary_print_recent_activity()
          line 26 of \blocks\recent_activity\block_recent_activity.php: call to print_recent_activity()
          line 280 of \blocks\moodleblock.class.php: call to block_recent_activity->get_content()
          line 232 of \blocks\moodleblock.class.php: call to block_base->formatted_contents()
          line 926 of \lib\blocklib.php: call to block_base->get_content_for_output()
          line 978 of \lib\blocklib.php: call to block_manager->create_block_contents()
          line 349 of \lib\blocklib.php: call to block_manager->ensure_content_created()
          line 7 of \theme\base\layout\general.php: call to block_manager->region_has_content()
          line 685 of \lib\outputrenderers.php: call to include()
          line 637 of \lib\outputrenderers.php: call to core_renderer->render_page_layout()
          line ? of unknownfile: call to core_renderer->header()
          line 1363 of \lib\setuplib.php: call to call_user_func_array()
          line 196 of \course\view.php: call to bootstrap_renderer->__call()
          line 196 of \course\view.php: call to bootstrap_renderer->header()
          

          The same error appeared when I logged in as a guest and then removed guest read permission.

          Show
          Michael de Raadt added a comment - Test result: failed. It would be good to see some "how to" in a couple of steps there, such as step 8. At least you should direct the tester to where this change should be made. In the glossary settings, Autolinking needs to be turned on and the RSS settings have to change. When I turned off student permission to read glossary entries and returned to the course page I was presented with the following error. I was controlling the permission at the course level. Debug info: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY ge.timemodified ASC LIMIT 0, 51' at line 3 SELECT ge.id, ge.concept, ge.approved, ge.timemodified, ge.glossaryid, u.id AS userid,u.picture,u.firstname,u.lastname,u.imagealt,u.email FROM mdl_glossary_entries ge JOIN mdl_user u ON u.id = ge.userid WHERE ge.timemodified > ? AND () ORDER BY ge.timemodified ASC LIMIT 0, 51 [array ( 0 => 1323664300, )] Stack trace: line 394 of \lib\dml\moodle_database.php: dml_read_exception thrown line 809 of \lib\dml\mysqli_native_moodle_database.php: call to moodle_database->query_end() line 380 of \mod\glossary\lib.php: call to mysqli_native_moodle_database->get_records_sql() line 1021 of \course\lib.php: call to glossary_print_recent_activity() line 26 of \blocks\recent_activity\block_recent_activity.php: call to print_recent_activity() line 280 of \blocks\moodleblock.class.php: call to block_recent_activity->get_content() line 232 of \blocks\moodleblock.class.php: call to block_base->formatted_contents() line 926 of \lib\blocklib.php: call to block_base->get_content_for_output() line 978 of \lib\blocklib.php: call to block_manager->create_block_contents() line 349 of \lib\blocklib.php: call to block_manager->ensure_content_created() line 7 of \theme\base\layout\general.php: call to block_manager->region_has_content() line 685 of \lib\outputrenderers.php: call to include() line 637 of \lib\outputrenderers.php: call to core_renderer->render_page_layout() line ? of unknownfile: call to core_renderer->header() line 1363 of \lib\setuplib.php: call to call_user_func_array() line 196 of \course\view.php: call to bootstrap_renderer->__call() line 196 of \course\view.php: call to bootstrap_renderer->header() The same error appeared when I logged in as a guest and then removed guest read permission.
          Hide
          Aparup Banerjee added a comment -

          hm, if we revert this , +1 to beautify the capability as Eloy mentioned.

          Show
          Aparup Banerjee added a comment - hm, if we revert this , +1 to beautify the capability as Eloy mentioned.
          Hide
          Aparup Banerjee added a comment -

          reopening this for Jason to fix Approvals array and rename capability.

          Show
          Aparup Banerjee added a comment - reopening this for Jason to fix Approvals array and rename capability.
          Hide
          Jason Fowler added a comment - - edited

          Eloy: I've made the name change for the capability while I had the code back for fixes, thank you for the suggestion on that, view is a much nicer name
          Michael: That error was triggered by my changes, but not caused by it, I have patched it for 2.3dev and 2.2 as part of this issue, but will back port the problem for 2.0 and 2.1

          Show
          Jason Fowler added a comment - - edited Eloy: I've made the name change for the capability while I had the code back for fixes, thank you for the suggestion on that, view is a much nicer name Michael: That error was triggered by my changes, but not caused by it, I have patched it for 2.3dev and 2.2 as part of this issue, but will back port the problem for 2.0 and 2.1
          Hide
          Jason Fowler added a comment -

          Thanks for testing, all ready for re-integration. Just improved the testing instructions too

          Show
          Jason Fowler added a comment - Thanks for testing, all ready for re-integration. Just improved the testing instructions too
          Hide
          Aparup Banerjee added a comment -

          Thanks for that , i've reverted the Jason's commits and integrated his commit at
          https://github.com/phalacee/moodle/commit/11157495ea5d28aaf2fb471aeab2191a5447de9e into 2.2 and master.

          reverts:
          2.2: b23dd6ee7257283dfc3457dddeccda938be734d0
          master: d79de196c9f0d19b54454e5109bb6b8ab34f07eb

          please test again.

          Show
          Aparup Banerjee added a comment - Thanks for that , i've reverted the Jason's commits and integrated his commit at https://github.com/phalacee/moodle/commit/11157495ea5d28aaf2fb471aeab2191a5447de9e into 2.2 and master. reverts: 2.2: b23dd6ee7257283dfc3457dddeccda938be734d0 master: d79de196c9f0d19b54454e5109bb6b8ab34f07eb please test again.
          Hide
          Michael de Raadt added a comment -

          Encountered the following errors:

          Notice: Undefined variable: approval in D:\xampp\htdocs\moodle_testing\mod\glossary\lib.php on line 378
          Notice: Undefined variable: approvalsql in D:\xampp\htdocs\moodle_testing\mod\glossary\lib.php on line 382
          
          Show
          Michael de Raadt added a comment - Encountered the following errors: Notice: Undefined variable: approval in D:\xampp\htdocs\moodle_testing\mod\glossary\lib.php on line 378 Notice: Undefined variable: approvalsql in D:\xampp\htdocs\moodle_testing\mod\glossary\lib.php on line 382
          Hide
          Jason Fowler added a comment -

          fixed and resubmitted

          Show
          Jason Fowler added a comment - fixed and resubmitted
          Hide
          Aparup Banerjee added a comment -

          Jason, please test the patch that you've updated and report the results here.

          Show
          Aparup Banerjee added a comment - Jason, please test the patch that you've updated and report the results here.
          Hide
          Jason Fowler added a comment -

          Another bug encountered - this was caused by the changes I made after discussing the bug with Ankit. I have looked into it deeper, and on second inspection, found that because the capability cannot be set on a per-glossary basis (it can only be set on a per course basis) the changes he suggested are not necessary, and in fact make the capability ineffective for guest users. I have returned that part of the code to the way it was before, and will re-do the whole battery of tests myself

          Show
          Jason Fowler added a comment - Another bug encountered - this was caused by the changes I made after discussing the bug with Ankit. I have looked into it deeper, and on second inspection, found that because the capability cannot be set on a per-glossary basis (it can only be set on a per course basis) the changes he suggested are not necessary, and in fact make the capability ineffective for guest users. I have returned that part of the code to the way it was before, and will re-do the whole battery of tests myself
          Hide
          Jason Fowler added a comment -

          This issue needs the capability extended a little further...
          Aparup: Please pull it out of integration so I can continue working on it at a more sane pace.

          Show
          Jason Fowler added a comment - This issue needs the capability extended a little further... Aparup: Please pull it out of integration so I can continue working on it at a more sane pace.
          Hide
          Ankit Agarwal added a comment -

          After discussing this with Jason, it was confirmed that Caps can be set on per glossary basis and the above mentioned condition was actually an issue.

          Show
          Ankit Agarwal added a comment - After discussing this with Jason, it was confirmed that Caps can be set on per glossary basis and the above mentioned condition was actually an issue.
          Hide
          Aparup Banerjee added a comment -

          Thanks for testing your patch Jason. (see it does help )

          Please discuss here or/and in developer chat about how we can come to a proper solution for this. verbosity+++!

          I've reverted the following commits.
          2.2.x:
          03adb289ec8396e15f18cbb45dddca1e834ed1c9
          b2b9890d08464710d652ecfa6c8f2910f9159593

          master:
          b92ac8c07989a7a7d8659169d465341e55a95d72
          aa423bd0e6a7b6c1f6cee4f95b46300e412551b4

          ps: i hearby so solemnly swear to never integrate issues right before a bowlarama. </joke>

          Show
          Aparup Banerjee added a comment - Thanks for testing your patch Jason. (see it does help ) Please discuss here or/and in developer chat about how we can come to a proper solution for this. verbosity+++! I've reverted the following commits. 2.2.x: 03adb289ec8396e15f18cbb45dddca1e834ed1c9 b2b9890d08464710d652ecfa6c8f2910f9159593 master: b92ac8c07989a7a7d8659169d465341e55a95d72 aa423bd0e6a7b6c1f6cee4f95b46300e412551b4 ps: i hearby so solemnly swear to never integrate issues right before a bowlarama. </joke>
          Hide
          Jason Fowler added a comment -

          Added the extra controls to the course/lib.php to prevent the glossaries from even showing up when the use does not have the capability to see them (in both the recent activity block for course changes and on the course index page itself)

          Show
          Jason Fowler added a comment - Added the extra controls to the course/lib.php to prevent the glossaries from even showing up when the use does not have the capability to see them (in both the recent activity block for course changes and on the course index page itself)
          Hide
          Jason Fowler added a comment -

          Updated the testing instructions with details on the new additions

          Show
          Jason Fowler added a comment - Updated the testing instructions with details on the new additions
          Hide
          Ankit Agarwal added a comment -

          Hi Jason,

          • Was just looking at the behavior of assignment module. Even if a user doesnot have read permissions to the assignments, he still can see it in the course index page.IMHO we should keep this behavior consistent in glossaries as well.
          • Afaik we use following naming conventions for contexts
            $coursecontext = get_context_instance(CONTEXT_COURSE, $courseid);
            

          Rest looks good
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jason, Was just looking at the behavior of assignment module. Even if a user doesnot have read permissions to the assignments, he still can see it in the course index page.IMHO we should keep this behavior consistent in glossaries as well. Afaik we use following naming conventions for contexts $coursecontext = get_context_instance(CONTEXT_COURSE, $courseid); Rest looks good Thanks
          Hide
          Michael de Raadt added a comment -

          Carrying this over to the next STABLE Sprint.

          Show
          Michael de Raadt added a comment - Carrying this over to the next STABLE Sprint.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, just a side comment, after having being thinking a bit about this issue:

          1) Note: In general, copying anything from the database module is not a good idea (IMO).

          2) Question: I've read the the description a bunch of times and still don't fully get the reason for bringing here a new capability. If it's somehow originated by rss needs... should't it better to be something to add and check by the rss subsystem? Sort of: mod/rss:view capability (just describing, surely that name is not ok). And then implement it so all module rss feeds (glossary/database/forum...) use them from core.

          3) Impact: Is that new planned capability going to affect general access to the module? Note that we have not such a capability in general for other modules (if I'm not wrong, only resource-like ones have that). Perhaps the alternative would be, and only for master, to implement such a cap globally, for all activities, perhaps moving all them to a common mod/view capability, used by all modules? Just guessing, and implement that as a core feature and not "each module its own one".

          Note that 2) and 3) are two alternatives that would lead to a more homogeneous handling of rss access and module access. But I don't think any of them are suitable for stable branches. Perhaps we should reconsider this and make a 2.3 project once planned properly?

          Just my 2 cents, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, just a side comment, after having being thinking a bit about this issue: 1) Note: In general, copying anything from the database module is not a good idea (IMO). 2) Question: I've read the the description a bunch of times and still don't fully get the reason for bringing here a new capability. If it's somehow originated by rss needs... should't it better to be something to add and check by the rss subsystem? Sort of: mod/rss:view capability (just describing, surely that name is not ok). And then implement it so all module rss feeds (glossary/database/forum...) use them from core. 3) Impact: Is that new planned capability going to affect general access to the module? Note that we have not such a capability in general for other modules (if I'm not wrong, only resource-like ones have that). Perhaps the alternative would be, and only for master, to implement such a cap globally, for all activities, perhaps moving all them to a common mod/view capability, used by all modules? Just guessing, and implement that as a core feature and not "each module its own one". Note that 2) and 3) are two alternatives that would lead to a more homogeneous handling of rss access and module access. But I don't think any of them are suitable for stable branches. Perhaps we should reconsider this and make a 2.3 project once planned properly? Just my 2 cents, ciao
          Hide
          Michael de Raadt added a comment -

          Hi, Eloy.

          I think we need to have the granularity to control how modules control access to RSS feeds. I'm not sure how many modules implement this, but I think it's needed and should be passive enough to backport, although that's not necessary.

          Show
          Michael de Raadt added a comment - Hi, Eloy. I think we need to have the granularity to control how modules control access to RSS feeds. I'm not sure how many modules implement this, but I think it's needed and should be passive enough to backport, although that's not necessary.
          Hide
          Jason Fowler added a comment -

          Ankit, I've made the changes you've recommended. Just going to run through the tests again myself to make sure that it all works as expected

          Show
          Jason Fowler added a comment - Ankit, I've made the changes you've recommended. Just going to run through the tests again myself to make sure that it all works as expected
          Hide
          Ankit Agarwal added a comment -

          Hi Jason,
          Looks good to me!
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jason, Looks good to me! Thanks
          Hide
          Jason Fowler added a comment -

          As per your suggestion in the devchat, I will update the context instance calls to the new API, and avoid the older function calls

          Show
          Jason Fowler added a comment - As per your suggestion in the devchat, I will update the context instance calls to the new API, and avoid the older function calls
          Hide
          Jason Fowler added a comment -

          All changes made and ready for integration

          Show
          Jason Fowler added a comment - All changes made and ready for integration
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I am reopening this issue sorry, we've had a bit of a discussion about it in the office chat and I think we really need to look at what this issue is hoping to achieve and how we can best go about resolving it.
          To help I'm just going to quote out a couple of things from the chat:

          My thoughts:

          (20/02/12 17:47:22) samhemelryk: Hmm I think we may need to discuss http://tracker.moodle.org/browse/MDL-30482
          (20/02/12 17:47:32) samhemelryk: The more I look at that the worse I think the idea is
          (20/02/12 17:50:22) samhemelryk: Capabilities are for controlling access across roles, so with this new view capability you could allow teachers access to the module and prevent students from accessing it, this would allow the teacher to view the RSS feed for the module and prevent the students, but as students couldn't access it nothing would change so there would be no point in having the RSS feed... not to mention in that situation you have re-invented the hidden field in the most complex way
          (20/02/12 17:51:24) samhemelryk: In reverse if you wanted to allow students to view the module + rss but not the teacher than you could do it with this new capability... or the hidden capability + field.. its all a little redundant
          (20/02/12 17:51:48) samhemelryk: Specifically in regards to this new capability I would need to see a very well thought out use case before I would ever consider giving it my +1
          (20/02/12 17:52:45) samhemelryk: I think Eloy hit the nail on the head when suggesting we look at implementing a generic capability to view the RSS feed that could then be overridden for the module by the admin/manager/teacher
          (20/02/12 17:52:53) samhemelryk: That is what this issue sounds like it actually wants
          (20/02/12 17:53:11) samhemelryk: Then you could prevent students from accessing the RSS but still allowing them to access the module
          (20/02/12 17:53:40) samhemelryk: Presently that issue gets a -1 from me purely based upon its concept

          Eloy's:

          (20/02/12 23:06:49) Cindereloy: I've to agree with Sam. I don't get the "pedagogical" base beyond inventing that alternative access-control.
          (20/02/12 23:07:48) Cindereloy: If the purpose is, exclusively, to control access to RSS feeds, then let's create one general (shared) cap for that. But one capability to control access to entries in glossary sounds 100% illogical here.
          (21/02/12 01:25:33) Cindereloy: Note about the "view" capability. I just found this while reviewing some lesson issues: http://tracker.moodle.org/browse/MDL-14448
          (21/02/12 01:26:14) Cindereloy: So perhaps there is one reason to create those "view" capabilities after all (hiding contents to some roles).
          (21/02/12 01:27:26) Cindereloy: In any case, as said, it must be discussed and agreed in a global way. Not added for each activity separately. Please discuss that @ HQ.

          And Apu:

          (14:33:41) aparup: btw with the glossary issue: MDL-30482 - Jason is on leave atm in NZ , so don't expect any new patches/response from him soon.
          +1 "we look at implementing a generic capability to view the RSS feed that could then be overridden for the module by the admin/manager/teacher"
          certainly centralising and optional overiding sound like a better way.

          Jason once yourself and Michael dR are back in the office I think we should all have a chat about this and sort out a master plan for it.

          Cheer
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I am reopening this issue sorry, we've had a bit of a discussion about it in the office chat and I think we really need to look at what this issue is hoping to achieve and how we can best go about resolving it. To help I'm just going to quote out a couple of things from the chat: My thoughts: (20/02/12 17:47:22) samhemelryk: Hmm I think we may need to discuss http://tracker.moodle.org/browse/MDL-30482 (20/02/12 17:47:32) samhemelryk: The more I look at that the worse I think the idea is (20/02/12 17:50:22) samhemelryk: Capabilities are for controlling access across roles, so with this new view capability you could allow teachers access to the module and prevent students from accessing it, this would allow the teacher to view the RSS feed for the module and prevent the students, but as students couldn't access it nothing would change so there would be no point in having the RSS feed... not to mention in that situation you have re-invented the hidden field in the most complex way (20/02/12 17:51:24) samhemelryk: In reverse if you wanted to allow students to view the module + rss but not the teacher than you could do it with this new capability... or the hidden capability + field.. its all a little redundant (20/02/12 17:51:48) samhemelryk: Specifically in regards to this new capability I would need to see a very well thought out use case before I would ever consider giving it my +1 (20/02/12 17:52:45) samhemelryk: I think Eloy hit the nail on the head when suggesting we look at implementing a generic capability to view the RSS feed that could then be overridden for the module by the admin/manager/teacher (20/02/12 17:52:53) samhemelryk: That is what this issue sounds like it actually wants (20/02/12 17:53:11) samhemelryk: Then you could prevent students from accessing the RSS but still allowing them to access the module (20/02/12 17:53:40) samhemelryk: Presently that issue gets a -1 from me purely based upon its concept Eloy's: (20/02/12 23:06:49) Cindereloy: I've to agree with Sam. I don't get the "pedagogical" base beyond inventing that alternative access-control. (20/02/12 23:07:48) Cindereloy: If the purpose is, exclusively, to control access to RSS feeds, then let's create one general (shared) cap for that. But one capability to control access to entries in glossary sounds 100% illogical here. (21/02/12 01:25:33) Cindereloy: Note about the "view" capability. I just found this while reviewing some lesson issues: http://tracker.moodle.org/browse/MDL-14448 (21/02/12 01:26:14) Cindereloy: So perhaps there is one reason to create those "view" capabilities after all (hiding contents to some roles). (21/02/12 01:27:26) Cindereloy: In any case, as said, it must be discussed and agreed in a global way. Not added for each activity separately. Please discuss that @ HQ. And Apu: (14:33:41) aparup: btw with the glossary issue: MDL-30482 - Jason is on leave atm in NZ , so don't expect any new patches/response from him soon. +1 "we look at implementing a generic capability to view the RSS feed that could then be overridden for the module by the admin/manager/teacher" certainly centralising and optional overiding sound like a better way. Jason once yourself and Michael dR are back in the office I think we should all have a chat about this and sort out a master plan for it. Cheer Sam
          Hide
          Jason Fowler added a comment -

          Every one seems to be focusing on this as if it were purely for controlling RSS access. That is not the case at all. It's a complete glossary control. Auto-linked glossary keywords, glossary blocks, and glossaries themselves are all controlled by this capability.

          Plus this capability brings the Glossary module into line with other modules, and is a step towards a more universal method of preventing access to restricted content for this module.

          Show
          Jason Fowler added a comment - Every one seems to be focusing on this as if it were purely for controlling RSS access. That is not the case at all. It's a complete glossary control. Auto-linked glossary keywords, glossary blocks, and glossaries themselves are all controlled by this capability. Plus this capability brings the Glossary module into line with other modules, and is a step towards a more universal method of preventing access to restricted content for this module.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think the main problem we are seeing here is not your/the current glossary/view implementation itself, it may be accepted.. but the important question/discussion about if this should be implemented in a "module by module" basis, or make it one "global mod functionality/capability", so all modules will support it.

          For reference, it was introduced and semi-discussed @ http://moodle.org/mod/cvsadmin/view.php?conversationid=9611#c335351

          It needs more thoughts/decision yet... IMO. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I think the main problem we are seeing here is not your/the current glossary/view implementation itself, it may be accepted.. but the important question/discussion about if this should be implemented in a "module by module" basis, or make it one "global mod functionality/capability", so all modules will support it. For reference, it was introduced and semi-discussed @ http://moodle.org/mod/cvsadmin/view.php?conversationid=9611#c335351 It needs more thoughts/decision yet... IMO. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Key use case (balancing the decision to have module-by-module caps:

          [13:11:44] <timhunt> Eloy, how do you define the roles for the prisonners use case?
          [13:11:45] <sam marshall> eloy - key factor - across the system we want people with a certain role to be prevented from using forum

          That only can be achieved with multiple.

          Show
          Eloy Lafuente (stronk7) added a comment - Key use case (balancing the decision to have module-by-module caps: [13:11:44] <timhunt> Eloy, how do you define the roles for the prisonners use case? [13:11:45] <sam marshall> eloy - key factor - across the system we want people with a certain role to be prevented from using forum That only can be achieved with multiple.
          Hide
          moodle.com added a comment -

          It looks like this is another reason to go with the module-by-module solution, which is where the current solution to this issue is at.

          I think there may also be some issues with a universal solution that we have not yet anticipated.

          Michael.

          Show
          moodle.com added a comment - It looks like this is another reason to go with the module-by-module solution, which is where the current solution to this issue is at. I think there may also be some issues with a universal solution that we have not yet anticipated. Michael.
          Hide
          Jason Fowler added a comment -

          Pushing this for integration in the hopes that everyone agrees that it should be done this way now...

          Show
          Jason Fowler added a comment - Pushing this for integration in the hopes that everyone agrees that it should be done this way now...
          Hide
          Jason Fowler added a comment -

          rebased and fixed version.php conflicts

          Show
          Jason Fowler added a comment - rebased and fixed version.php conflicts
          Hide
          Petr Škoda added a comment -

          to integrators 1: please review capability defaults, I guess it breaks very badly on the frontpage (see other resource modules)
          to integrators 2: please review relaxed access control in rsslib, something like require_login should be mandatory there

          Show
          Petr Škoda added a comment - to integrators 1: please review capability defaults, I guess it breaks very badly on the frontpage (see other resource modules) to integrators 2: please review relaxed access control in rsslib, something like require_login should be mandatory there
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Petr Škoda added a comment -

          to integrators: see my two points above, they were not resolved yet

          Show
          Petr Škoda added a comment - to integrators: see my two points above, they were not resolved yet
          Hide
          Jason Fowler added a comment -

          Petr, they have not been resolved because you aren't explaining what needs to be resolved to the person who needs to resolve it. Addressing your concerns to the integrators rather than the developer is rather passive aggressive. Please simply explain to me what you think needs to be changed.

          Show
          Jason Fowler added a comment - Petr, they have not been resolved because you aren't explaining what needs to be resolved to the person who needs to resolve it. Addressing your concerns to the integrators rather than the developer is rather passive aggressive. Please simply explain to me what you think needs to be changed.
          Hide
          Jason Fowler added a comment -

          What good is an RSS feed if you have to be logged in to access it? Most feed readers don't offer that sort of ability - they simply read the feed, if it redirects to something that isn't a feed, it fails

          Show
          Jason Fowler added a comment - What good is an RSS feed if you have to be logged in to access it? Most feed readers don't offer that sort of ability - they simply read the feed, if it redirects to something that isn't a feed, it fails
          Hide
          Martin Dougiamas added a comment -

          Petr's point 1 is just about the defaults for the new capability. On a vanilla site, try to add a glossary to the front page with entries and a feed and see if users can see the RSS feed and if it contains entries.

          The "login" for RSS is the token that is in the URL ... that should always be checked to be valid and should give you a valid user. Is that happening for glossaries as well? If so, that's enough.

          Show
          Martin Dougiamas added a comment - Petr's point 1 is just about the defaults for the new capability. On a vanilla site, try to add a glossary to the front page with entries and a feed and see if users can see the RSS feed and if it contains entries. The "login" for RSS is the token that is in the URL ... that should always be checked to be valid and should give you a valid user. Is that happening for glossaries as well? If so, that's enough.
          Hide
          Jason Fowler added a comment -

          We've discussed the RSS issue in Dev chat and come to the conclusion that the problem you have raised is unrelated to the issue here. A separate issue will need to be raised to address that.

          Show
          Jason Fowler added a comment - We've discussed the RSS issue in Dev chat and come to the conclusion that the problem you have raised is unrelated to the issue here. A separate issue will need to be raised to address that.
          Hide
          Martin Dougiamas added a comment - - edited

          After looking into it, I think rss_get_userid_from_token() in lib/rsslib.phpshould definitely check the user is a valid current user (and not deleted etc)

          Show
          Martin Dougiamas added a comment - - edited After looking into it, I think rss_get_userid_from_token() in lib/rsslib.phpshould definitely check the user is a valid current user (and not deleted etc)
          Hide
          Jason Fowler added a comment -

          but is that really part of this issue, or a separate issue altogether?

          Show
          Jason Fowler added a comment - but is that really part of this issue, or a separate issue altogether?
          Hide
          Petr Škoda added a comment -

          Jason: I am sorry that I have offended you, it will not happen again.

          Thanks Martin for explaining the problems in more detail.

          Show
          Petr Škoda added a comment - Jason: I am sorry that I have offended you, it will not happen again. Thanks Martin for explaining the problems in more detail.
          Hide
          Jason Fowler added a comment -

          I'll be working on the RSS lib issue as a separate issue, can someone show me the front page issue though?

          Show
          Jason Fowler added a comment - I'll be working on the RSS lib issue as a separate issue, can someone show me the front page issue though?
          Hide
          Jason Fowler added a comment -

          Thanks to Ankit, I have been able to solve the frontpage issue, and now the issue can proceed with integration

          Show
          Jason Fowler added a comment - Thanks to Ankit, I have been able to solve the frontpage issue, and now the issue can proceed with integration
          Hide
          Aparup Banerjee added a comment - - edited

          Hello,

          looks like (1) frontpage missing cap that Petr mentioned is resolved. (and its added to test too, thanks Jason)

          for (2) ,i've created MDL-32128.

          This has been integrated to master. I've resolved some conflicts and whitespace errors. (Jason, there was a tab that didn't convert to spaces, you might need to check your editor's settings)

          Tester: perhaps also test an upgrade and also play with roles and the permissions here with several modules (glossary + others).

          Show
          Aparup Banerjee added a comment - - edited Hello, looks like (1) frontpage missing cap that Petr mentioned is resolved. (and its added to test too, thanks Jason) for (2) ,i've created MDL-32128. This has been integrated to master. I've resolved some conflicts and whitespace errors. (Jason, there was a tab that didn't convert to spaces, you might need to check your editor's settings) Tester: perhaps also test an upgrade and also play with roles and the permissions here with several modules (glossary + others).
          Hide
          Michael de Raadt added a comment -

          Cross your fingers.

          Show
          Michael de Raadt added a comment - Cross your fingers.
          Hide
          Michael de Raadt added a comment -

          I ran into a problem. A guest user is able to see "New glossary entries" in Recent activity block. Apu is looking to see if he can affect a quick fix.

          Show
          Michael de Raadt added a comment - I ran into a problem. A guest user is able to see "New glossary entries" in Recent activity block. Apu is looking to see if he can affect a quick fix.
          Hide
          Michael de Raadt added a comment -

          Test result: Close, but not completely there.

          I tested this as a student, front page authenticated user and guest, with the "View glossary" capability set to Prevent. As guest I was able to see "New glossary entries" in the "Recent activity" block in a course and on the front page. As an authenticated user I was able to see "New glossary entries" on the front page, but not in a course.

          Apu had a quick look, but he had another similar request at the same time. It is possible that if this is fixed early tomorrow we could still get it into this week's integration.

          Show
          Michael de Raadt added a comment - Test result: Close, but not completely there. I tested this as a student, front page authenticated user and guest, with the "View glossary" capability set to Prevent. As guest I was able to see "New glossary entries" in the "Recent activity" block in a course and on the front page. As an authenticated user I was able to see "New glossary entries" on the front page, but not in a course. Apu had a quick look, but he had another similar request at the same time. It is possible that if this is fixed early tomorrow we could still get it into this week's integration.
          Hide
          Sam Hemelryk added a comment - - edited

          I've had a quick look at this in regards to the results Michael mentioned.
          Without testing and only having looked at the code for a second there is one blaring problem in the block changes.
          Within the block $this->context is the context of the block, and its parent will be where ever the block was initially added.
          The check in block_glossary_random::get_content is wrong in that it should be checking the users capability on the glossary context and not the users capability on the block.

          Edited: Sorry the retarded spellcheck in chrome is removing words it's fixed when I submit a form.. fixed up the

          Show
          Sam Hemelryk added a comment - - edited I've had a quick look at this in regards to the results Michael mentioned. Without testing and only having looked at the code for a second there is one blaring problem in the block changes. Within the block $this->context is the context of the block, and its parent will be where ever the block was initially added. The check in block_glossary_random::get_content is wrong in that it should be checking the users capability on the glossary context and not the users capability on the block. Edited: Sorry the retarded spellcheck in chrome is removing words it's fixed when I submit a form.. fixed up the
          Hide
          Jason Fowler added a comment -

          Thanks Sam, I didn't even know that would make a difference.

          Show
          Jason Fowler added a comment - Thanks Sam, I didn't even know that would make a difference.
          Hide
          Michael de Raadt added a comment -

          OK, so I went and checked again and it appears that recent glossary entries are shown to all users regardless of permissions. What I was seeing before was a product of the order in which I tested this.

          Show
          Michael de Raadt added a comment - OK, so I went and checked again and it appears that recent glossary entries are shown to all users regardless of permissions. What I was seeing before was a product of the order in which I tested this.
          Hide
          Jason Fowler added a comment -

          Thanks for correcting the comment, now I actually have a valid explanation for the issue, I can stop wasting time chasing ghosts...

          Show
          Jason Fowler added a comment - Thanks for correcting the comment, now I actually have a valid explanation for the issue, I can stop wasting time chasing ghosts...
          Hide
          Jason Fowler added a comment -

          Okay, all the fixes have been pushed, just waiting on a quick test from ankit to make certain this will work before the final test by Michael

          Show
          Jason Fowler added a comment - Okay, all the fixes have been pushed, just waiting on a quick test from ankit to make certain this will work before the final test by Michael
          Hide
          Ankit Agarwal added a comment -

          Did a quick test of with a student user...results looks good!
          Thanks

          Show
          Ankit Agarwal added a comment - Did a quick test of with a student user...results looks good! Thanks
          Hide
          Jason Fowler added a comment -

          okay, ankit tested it and it's clear to be integrated again

          Show
          Jason Fowler added a comment - okay, ankit tested it and it's clear to be integrated again
          Hide
          Aparup Banerjee added a comment -

          ok another fix is now pushed up to master - that context should be right now.

          Show
          Aparup Banerjee added a comment - ok another fix is now pushed up to master - that context should be right now.
          Hide
          Michael de Raadt added a comment -

          Test result: Success. \o/

          Well done Jason.

          I rechecked this with and without permissions.

          Show
          Michael de Raadt added a comment - Test result: Success. \o/ Well done Jason. I rechecked this with and without permissions.
          Hide
          Sam Hemelryk added a comment -

          Congratulations are in order, you've made it, or at least your code has!
          It's now part of Moodle and both the git and cvs repositories have been updated.

          This issue is being marked as fixed and closed.

          Thank you.

          Show
          Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.
          Hide
          Helen Foster added a comment -

          Documentation on this improvement is now available:

          http://docs.moodle.org/23/en/Capabilities/mod/glossary:view

          Show
          Helen Foster added a comment - Documentation on this improvement is now available: http://docs.moodle.org/23/en/Capabilities/mod/glossary:view
          Hide
          Jason Fowler added a comment -

          Thanks Helen, I totally missed the docs_required on this, sorry about that.

          Show
          Jason Fowler added a comment - Thanks Helen, I totally missed the docs_required on this, sorry about that.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: