[Contrib: Evaluation System] Proposed changes to make hierarchy more useable

Fri Nov 4 17:04:54 PDT 2011

Since there were no further comments, I committed the patches. EVALSYS-1146
was revised to use makeEvalGroupObject.

Thanks,
Nicola

On Thu, Oct 27, 2011 at 4:02 PM, Nicola Monat-Jacobs
<nicola at longsight.com>wrote:

> Thanks for your questions Aaron - I'm very happy to have some discussion
> about this before committing anything.
>
> Re: EVALSYS-967
>
> Ah, I see, so originally, hardcoding in "admin" made sense, because it
> would return all groups. But now it doesn't - it only returns the groups
> 'admin' is explicitly enrolled in.
>
> Currently, the code does
> 1) Get all evalgroups for admin
> 2) See if any of those groups are checked
>
> Since #1 doesn't get all groups in the system (as Aaron indicates it once
> did), I basically propose changing the order up a bit:
> 1) Get all checked groups for this node, regardless of if admin or the
> current user has access to them. This clearly shows the current user what
> groups are in this node, without any restrictions.
> 2) Look at the current courses the user is enrolled in, eliminate any
> courses we've already listed in #1 and add the rest as unchecked. This
> allows the user to add additional courses.
>
> Ideally, instead of #2, we'd have an interface where the user could look
> up any course in the system and add it, but that's a bit more work and I
> feel this is a good interim measure.
>
> If the only change we made was to change "admin" to the current user, then
> we'd be making an assumption. We'd be assuming that someone has to have
> "EvalConstants.PERM_BE_EVALUATED" in a course before we can allow them to
> associate it to a point in the hierarchy or even see that this course is
> associated.
>
> Is that a valid assumption considering that this person is an evaluation
> administrator? If they are an administrator with access to "Administrate"
> and "Control Hierarchy" then why does this need to be enforced? I would
> argue it only puts up an unnecessary barrier to them doing their job.
>
> Unless there's another interface where this code is used besides in
> Administrate > Control Hierarchy?
>
> I've added this as a comment to the JIRA too.
>
> Re: EVALSYS-1146
>
> I wasn't aware of EvalCommonLogic.makeEvalGroupObject - I'll see if I can
> use that instead.
>
> Thanks,
> Nicola
>
> On Wed, Oct 26, 2011 at 6:29 AM, Aaron Zeckoski <azeckoski at unicon.net>wrote:
>
>> There seems to be an unnecessary change in
>> https://jira.sakaiproject.org/browse/EVALSYS-967
>>
>> The point of the locateBean method is to find all the groups the user
>> should be able to deal with and then show them as checked if they were
>> already assigned or unchecked if they were not. Since the method
>> getEvalGroupsForUser was changed awhile back to no longer return all
>> groups when the user is an admin (I forget who made this change), the
>> code doesn't quite work the way it used to. This is why this change to
>> line 64 is now necessary.
>> However, for some reason, the checked/unchecked filtering was changed
>> as well and I don't see why that part of the change is necessary.
>> Maybe some extra explanation in the ticket would help.
>>
>>
>> https://jira.sakaiproject.org/browse/EVALSYS-1146
>> does not follow best practices and uses a sakai interface directly
>> (SiteService) instead of using EvalCommonLogic.makeEvalGroupObject but
>> in the case of the UI it is probably tolerable though I would prefer
>> it not be the case. Otherwise it looks like it is probably OK (though
>> a bit slow for large numbers of groups).
>>
>>
>> -AZ
>>
>>
>>
>> On Tue, Oct 25, 2011 at 4:09 PM, Nicola Monat-Jacobs
>> <nicola at longsight.com> wrote:
>> > Thanks Aaron. It looks like EVALSYS-1146 did actually need a patch
>> also. If
>> > you have a sec to review it, that would be awesome.
>> > Thanks!
>> >
>> > On Mon, Oct 24, 2011 at 5:03 PM, Aaron Zeckoski <azeckoski at unicon.net>
>> > wrote:
>> >>
>> >> Sounds good to me.
>> >> It does circumvent some possible security but I think that is probably
>> OK.
>> >> -AZ
>> >>
>> >>
>> >> On Mon, Oct 24, 2011 at 7:45 PM, Nicola Monat-Jacobs
>> >> <nicola at longsight.com> wrote:
>> >> > There are two outstanding issues with the hierarchy interface that I
>> >> > want to
>> >> > run by everyone.
>> >> > https://jira.sakaiproject.org/browse/EVALSYS-967
>> >> > This has been an annoyance for a while - it means that you can only
>> >> > assign a
>> >> > course site (or 'evalgroup' as it is in know in evalsys) to a
>> position
>> >> > in
>> >> > the hierarchy if the 'admin' user is enrolled in that course site.
>> This
>> >> > means that admin has to be enrolled in all your course sites if you
>> want
>> >> > to
>> >> > use hierarchy.
>> >> > https://jira.sakaiproject.org/browse/EVALSYS-1146
>> >> > Additionally, if you are successful in assigning your nodes through
>> some
>> >> > other means, those assignments won't always show in the GUI to the
>> >> > current
>> >> > user (confusing! user wants to be sure eval will go to the courses
>> they
>> >> > think it will). Even if we just swapped admin for currentUser (to
>> >> > address
>> >> > EVALSYS-967 above) the assigned courses won't show if the current
>> user
>> >> > isn't
>> >> > enrolled in them.
>> >> > So, the proposed solution for EVALSYS-967 addresses both these
>> >> > solutions.
>> >> > Instead of looking up all the sites 'admin' is enrolled in, and then
>> >> > finding
>> >> > out which of those are or are not assigned to this node, we go
>> >> > a conceptually simpler route.
>> >> > The proposed patch looks up and shows the sites currently assigned to
>> >> > this
>> >> > node (regardless of if the current user, or admin, is enrolled in
>> them).
>> >> > Then, it looks up and shows the courses that the current user (not
>> >> > admin) is
>> >> > enrolled in and offers those up to be added to the current node.
>> >> > A more robust solution would allow the user to have some sort of
>> lookup
>> >> > interface that would allow them to search for any course in the
>> system,
>> >> > but
>> >> > I think this is a good interim measure to allow folks to actually use
>> >> > this
>> >> > feature.
>> >> > Thoughts?
>> >> > - Nicola
>> >> > _______________________________________________
>> >> > evaluation mailing list
>> >> > evaluation at collab.sakaiproject.org
>> >> > http://collab.sakaiproject.org/mailman/listinfo/evaluation
>> >> >
>> >> > TO UNSUBSCRIBE: send email to
>> >> > evaluation-unsubscribe at collab.sakaiproject.org
>> >> > with a subject of "unsubscribe"
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >> Aaron Zeckoski - Software Architect - http://tinyurl.com/azprofile
>> >
>> >
>>
>>
>>
>> --
>> Aaron Zeckoski - Software Architect - http://tinyurl.com/azprofile
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://collab.sakaiproject.org/pipermail/evaluation/attachments/20111104/79364cd4/attachment.html 