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

Hidden teachers shown in Choice

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.3
    • Fix Version/s: 1.9
    • Component/s: Choice, Roles / Access
    • Labels:
      None
    • Environment:
      Redhat
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      I have added a Choice with the settings "Always show results to students" and "Publish full results, showing names and their choices" which shows all participants, students can see hidden teachers too. No students have the viewhiddenassigns capability, so I assume this is a bug.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              lazyfish Yu Zhang added a comment -

              Should be fixed now, thanks for the report.

              Show
              lazyfish Yu Zhang added a comment - Should be fixed now, thanks for the report.
              Hide
              benteo Bente Olsen added a comment -

              Thanks a lot!

              Show
              benteo Bente Olsen added a comment - Thanks a lot!
              Hide
              danmarsden Dan Marsden added a comment -

              I'm not convinced on this patch... - my feeling would be that if a "hidden" user selected a "choice" then their selection should be shown. - this potentially screws up the limit function available in the choice.

              (of course they shouldn't be shown in the unanswered column...)

              did you test this patch with the limit in the choice?

              thanks!

              Dan

              Show
              danmarsden Dan Marsden added a comment - I'm not convinced on this patch... - my feeling would be that if a "hidden" user selected a "choice" then their selection should be shown. - this potentially screws up the limit function available in the choice. (of course they shouldn't be shown in the unanswered column...) did you test this patch with the limit in the choice? thanks! Dan
              Hide
              lazyfish Yu Zhang added a comment -

              Hi Dan,

              The limit is unmodified and will work as before, hidden teacher's choices are recorded, but if you are a student with no viewhidden capability then you do not see the listing of that hidden teacher's result on display. Teachers with viewhidden capability will be able to see all user's choices. I think this should be the correct behaviour. Is there a specific scenario that this would be bad?

              Cheers,

              Yu

              Show
              lazyfish Yu Zhang added a comment - Hi Dan, The limit is unmodified and will work as before, hidden teacher's choices are recorded, but if you are a student with no viewhidden capability then you do not see the listing of that hidden teacher's result on display. Teachers with viewhidden capability will be able to see all user's choices. I think this should be the correct behaviour. Is there a specific scenario that this would be bad? Cheers, Yu
              Hide
              danmarsden Dan Marsden added a comment -

              I haven't had a chance to test it yet, but can you check the following scenario please?

              set up a choice with the limit allowing 1 selection to option X - log in as a hidden teacher, and select that option.
              then login as a normal student, and check to make sure you can't select that option, as it should be full.

              thanks!

              Dan

              Show
              danmarsden Dan Marsden added a comment - I haven't had a chance to test it yet, but can you check the following scenario please? set up a choice with the limit allowing 1 selection to option X - log in as a hidden teacher, and select that option. then login as a normal student, and check to make sure you can't select that option, as it should be full. thanks! Dan
              Hide
              danmarsden Dan Marsden added a comment -

              this patch doesn't really seem to have "fixed" a bug, rather introduced one. - Yu - did you test this properly?

              Show
              danmarsden Dan Marsden added a comment - this patch doesn't really seem to have "fixed" a bug, rather introduced one. - Yu - did you test this properly?
              Hide
              danmarsden Dan Marsden added a comment -

              I can't back this change out as I haven't got access to the new CVS server yet - If you get a chance, can you please back this change out to all versions that you have patched?

              if an admin selects a choice, if you click on "view responses" (only available to admins) - the admin user is not shown like it should be. (I'm guessing hidden users aren't shown either.)

              Show
              danmarsden Dan Marsden added a comment - I can't back this change out as I haven't got access to the new CVS server yet - If you get a chance, can you please back this change out to all versions that you have patched? if an admin selects a choice, if you click on "view responses" (only available to admins) - the admin user is not shown like it should be. (I'm guessing hidden users aren't shown either.)
              Hide
              mudrd8mz David Mudrák added a comment -

              I agree with Dan. A user (let us say teacher) enroled by a hidden assignment may stay invisible to the rest of participants, but only unless (s)he does not submit something into the course.

              Show
              mudrd8mz David Mudrák added a comment - I agree with Dan. A user (let us say teacher) enroled by a hidden assignment may stay invisible to the rest of participants, but only unless (s)he does not submit something into the course.
              Hide
              danmarsden Dan Marsden added a comment -

              I've posted a possible solution to this in MDL-12890 which involves changes to acceslib.php function get_users_by_capability

              this code:
              $from = " FROM {$CFG->prefix}user u
              JOIN (SELECT DISTINCT ssra.userid,
              FROM {$CFG->prefix}role_assignments ssra
              WHERE ssra.contextid IN ($ctxids)
              AND ssra.roleid IN (".implode(',',$roleids) .")
              $sscondhiddenra
              ) ra ON ra.userid = u.id
              $uljoin ";

              by changing to this:
              $from = " FROM {$CFG->prefix}user u
              JOIN (SELECT DISTINCT ssra.userid, ssra.hidden
              FROM {$CFG->prefix}role_assignments ssra
              WHERE ssra.contextid IN ($ctxids)
              AND ssra.roleid IN (".implode(',',$roleids) .")
              $sscondhiddenra
              ) ra ON ra.userid = u.id
              $uljoin ";

              and then when calling get users_by_capability adding 'hidden' to the $fields list

              Show
              danmarsden Dan Marsden added a comment - I've posted a possible solution to this in MDL-12890 which involves changes to acceslib.php function get_users_by_capability this code: $from = " FROM {$CFG->prefix}user u JOIN (SELECT DISTINCT ssra.userid, FROM {$CFG->prefix}role_assignments ssra WHERE ssra.contextid IN ($ctxids) AND ssra.roleid IN (".implode(',',$roleids) .") $sscondhiddenra ) ra ON ra.userid = u.id $uljoin "; by changing to this: $from = " FROM {$CFG->prefix}user u JOIN (SELECT DISTINCT ssra.userid, ssra.hidden FROM {$CFG->prefix}role_assignments ssra WHERE ssra.contextid IN ($ctxids) AND ssra.roleid IN (".implode(',',$roleids) .") $sscondhiddenra ) ra ON ra.userid = u.id $uljoin "; and then when calling get users_by_capability adding 'hidden' to the $fields list
              Hide
              danmarsden Dan Marsden added a comment -

              I'm finding it hard to find an easy way of finding if a user assigned to a capability is a given their role via a hidden assignment.....

              I 'could' call get_users_by capability once including hidden users, and then a 2nd time without, and compare the 2 arrays, but that would take a performance hit!

              I need to return all hidden users on the first call, as hidden users should be counted in the results, but not displayed in the list of unanswered users.

              Ideally it would be good if get_users_by_capability returned a variable for each user stating if their role assignment was hidden or not.... - see the hack posted in the comment above for an initial example

              Adding MartinL and Skodak to this - do you guys have any ideas how I might be able to do this? - is there a way without modifying accesslib?

              thanks!

              Show
              danmarsden Dan Marsden added a comment - I'm finding it hard to find an easy way of finding if a user assigned to a capability is a given their role via a hidden assignment..... I 'could' call get_users_by capability once including hidden users, and then a 2nd time without, and compare the 2 arrays, but that would take a performance hit! I need to return all hidden users on the first call, as hidden users should be counted in the results, but not displayed in the list of unanswered users. Ideally it would be good if get_users_by_capability returned a variable for each user stating if their role assignment was hidden or not.... - see the hack posted in the comment above for an initial example Adding MartinL and Skodak to this - do you guys have any ideas how I might be able to do this? - is there a way without modifying accesslib? thanks!
              Hide
              martinlanghoff Martín Langhoff added a comment -

              Dan - this is somewhat tricky for 2 reasons:

              • the hidden role assignment might deny/prohibit the capability!
              • in practice, it's tricky to "blame" a specific RA for a capability the user has.

              is that SQL from 1.8? Have a look at 1.9 to see the scope of the problem.

              Show
              martinlanghoff Martín Langhoff added a comment - Dan - this is somewhat tricky for 2 reasons: the hidden role assignment might deny/prohibit the capability! in practice, it's tricky to "blame" a specific RA for a capability the user has. is that SQL from 1.8? Have a look at 1.9 to see the scope of the problem.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Martin, yeah - I did see the complexities - I was hoping it could be easier!

              that SQL is in 1.9 when there aren't any Negative Permissions......

              there must be a way of returning whether the user got the capability via a hidden role assignment when there are negative permissions! I definately don't want to call get_users_by_capability twice!

              The other option is to force it so that users given the "mod/choice:choose" capability via a hidden role assignment just can't submit a choice. But that would likely cause confusion as well!

              Show
              danmarsden Dan Marsden added a comment - Hi Martin, yeah - I did see the complexities - I was hoping it could be easier! that SQL is in 1.9 when there aren't any Negative Permissions...... there must be a way of returning whether the user got the capability via a hidden role assignment when there are negative permissions! I definately don't want to call get_users_by_capability twice! The other option is to force it so that users given the "mod/choice:choose" capability via a hidden role assignment just can't submit a choice. But that would likely cause confusion as well!
              Hide
              martinlanghoff Martín Langhoff added a comment -

              > that SQL is in 1.9 when there aren't any Negative Permissions

              hah! I knew it - that's pretty close to the original implementation in 1.7/1.8.

              I think it can be done, but it will take significant work on get_users_by_capability() or a separate function. Maybe call it twice as a temporary measure, while we try and sort this out? I don't want to block the release on this

              Show
              martinlanghoff Martín Langhoff added a comment - > that SQL is in 1.9 when there aren't any Negative Permissions hah! I knew it - that's pretty close to the original implementation in 1.7/1.8. I think it can be done, but it will take significant work on get_users_by_capability() or a separate function. Maybe call it twice as a temporary measure, while we try and sort this out? I don't want to block the release on this
              Hide
              danmarsden Dan Marsden added a comment -

              attaching patch which calls get_users_capabilities twice, and a couple of if statements to check against the 2nd array to see if they exist.

              thanks Martin.

              Show
              danmarsden Dan Marsden added a comment - attaching patch which calls get_users_capabilities twice, and a couple of if statements to check against the 2nd array to see if they exist. thanks Martin.
              Hide
              danmarsden Dan Marsden added a comment -

              oops - missed a bit - here's the right one!

              Show
              danmarsden Dan Marsden added a comment - oops - missed a bit - here's the right one!
              Hide
              danmarsden Dan Marsden added a comment -

              fix now in 1.9 and HEAD.

              Show
              danmarsden Dan Marsden added a comment - fix now in 1.9 and HEAD.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Mar/08