QA/QC column of yields listing works incorrectly

[gsrohde]

A user with "Creator" or "Viewer" page access level will see a select box for editing the "checked" status of a yield record if and only if his/her data access level is strictly less (i.e. more privileged) than the data access level of the yield being edited. (The exception to this is the case where the logged-in user is the one who created the yield record--users will always see the "checked" select box for records they themselves created. Note that "Viewers" can't actually change the "checked" status: Trying to change the value using the select box will silently fail for "Viewers".)

It seems to me that this should be "less than or equal to"--that is, a user who has the same data access level as that of the record being edited should be able to edit the "checked" status of that record (provided, of course, that they have at least "Creator" page access privileges; it would seem that "Viewers" shouldn't see the select boxes at all since they can't use them to change the attribute value).

@dlebauer Please comment on this.

A related problem is that when a user isn't sufficiently privileged to be presented with the select box, a fixed non-editable value will appear instead. This should represent the current value of the "checked" attribute, but the logic for this is wrong. The value displayed is "passed" if the actual value is unchecked, "failed" if the actual value is passed, and "unchecked" if the actual value is failed.

Related problem with the access level select box:

The display of the select box for access_level would work similarly except that there is another "OR" clause in the condition used to test whether or not to display the select box, and that clause has two syntax errors that make it always evaluate to "true".

The buggy clause is (current_user.access_level = y.access_level and y.checked). The "current_user.access_level = y.access_level" part is always true because = was used in place of ==, so the value of this sub-clause is equal to the value assigned—namely, y.access_level—and this is 1, 2, 3, or 4, all of which evaluate to "true". But actually, in Ruby, all numerical values are "true"—even zero and negative one—so the "y.checked" part of the clause is always true as well, making the whole clause true.

It would seem that the intended clause was (current_user.access_level == y.access_level and y.checked == 1), but the rationale for having this case is not clear to me. (With this correction, the entire test for whether to display the access_level select box then becomes "if ( current_user.access_level < y.access_level ) or ( current_user.access_level == y.access_level and y.checked == 1) or ( y.user_id == current_user.id ) or ( current_user.page_access_level <= 2 ); then ...".)

The upshot of this syntax error is that the access_level select box always appears in each row that appears. (Note that just as for the "checked" attribute, a users without at least "Creator" page access privileges can't actually use the select box to change the access level of a yield record. Note also that a more privileged user who has less than full data access can lock him/herself out of a yield record by changing the data access level of the record to one which is more restrictive than his/her own access level. As soon as the access level is changed and the index page is refreshed, the record will disappear for that user.)

@dlebauer Please comment on this also.

[dlebauer]

1. Yes, should be less than or equal to.
2. should be:
   • anyone can mark a record that they can view as -1 (current_user.access_level <= y.access_level and y.checked == 1)
   • anyone can mark their own records as unchecked.( y.user_id == current_user.id )
   • administrators and managers can change 'checked' status ( current_user.page_access_level <= 2 ); then ...".)

[gsrohde]

After implementing (1.) above and fixing a long-standing bug (see the commit message at 3703fbf), things will work as described below. I'm waiting to deal with (2.) until I am sure I understand it correctly.

Here are how things work now (or will work after code modifications have been deployed):

• Administrators can view all yields no matter what their data access level.
• Managers can view all yields that they themselves created and all yields that their data access level authorizes (i.e. yields.access_level >= current_user.access_level).
• Creators and Viewers can view all yields that they themselves created. They can also view all yields that their data access level authorizes provided the yield is marked as "checked".

Regarding ability to edit the access level from the index page:

• Administrators can edit the access level of any yield.
• Managers and Creators can edit the access level of any yield that they can view.
• Viewers can't edit the access level.

Note that these rules create the somewhat perverse result that a Creator may lock himself out of a record that he himself didn't create if he (1) changes the QA/QC status to something other than checked; (2) changes the access level to some level more restrictive than he has viewing permission for.

If I understand your remarks in (2.) correctly, what remains to be done is to prevent Creators from marking a record as checked and also prevent them from marking any records other than their own as unchecked (regardless of whether the previous status was checked or failed?). I'm assuming here that when you say "anyone", you don't include viewers, for then we would give up all control over who can change the database.

You don't deal in your comment with permissions for changing the access level, so I haven't made any changes yet to the way that works.

[gsrohde]

@dlebauer Another related question: Should the listing page for traits work exactly the same way?

[dlebauer]

Yes traits and yields should work the same way.

Only admins and managers should be able to change access level, though
creators should be able to set it on a new record, even if it locks them
out.

An exception is that creators should be able to view and edit any records
that they created.
On Thu, Jun 25, 2015 at 12:13 PM Scott Rohde notifications@github.com
wrote:

@dlebauer https://github.com/dlebauer Another related question: Should
the listing page for traits work exactly the same way?

—
Reply to this email directly or view it on GitHub
#303 (comment).