Add created_by to record list

[pcaisse]

Overview

Add "Created by" field to the record list.

Checklist

• PR has a descriptive enough title to be useful in changelogs

Screenshots

Testing Instructions

• Go to http://localhost:7000/#!/list and make sure the field shows up with appropriate values

Closes #159626646

[rmartz]

Looks good! Left a couple comments but most are informational, I think there may be a vestigial line to remove but that's the only real thing.

Couple things that I'm unsure about:

• It's not specified in the requirements for this one, but in PT159629145 it says

  The filter should assume that the user is inputting an email address [...], but fall back to username search if there is no user with that email address (because this is what the Record detail page does if a user has been deleted).

  I think the expectation is for the column to default to email and fallback to username, but it's not explicitly stated. The acceptance criteria for this specifies only for the email to be shown, but also has a note suggesting that username is available in cases where the user has been deleted.

• The AC specifies that the column should only be visible to users who are authenticated, but I don't see logic that looks like it'd control that, and I'm not familiar enough with the system to test as an unauthenticated user. Is there anything we need to do to prevent public users from seeing this?

[rmartz]

👍 Glad I'm not the only one who prefers this over stalactite style.

[rmartz]

For methods like all() the [...] list comprehension isn't necessary, you can pass the check in as a generator expression. It's slightly simpler, plus all() and any() can stop early if they fail whereas [...] always has to process the entire list.

[rmartz]

Is this needed? Also cls is conventionally used as the equivalent to self in @classmethod functions, so I'd avoid using it to refer to other classes to avoid confusion.

[pcaisse]

Woops. Yeah, I think this can be safely deleted.

[rmartz]

This seems interesting... what kind of situation would we need to sort the creating audit log entry? Is it possible for a record to have more than one?

[pcaisse]

I don't think it's possible but... anything's possible. I was on the fence about deleting this line or not, wondering what we should do if we did have two CREATE audit log entries.

I think we can probably just delete it. This shouldn't happen and this line probably just makes the code more confusing.

[rmartz]

Performance-wise for this kind of use it's probably basically equivalent and I'm not sure if it's necessarily clearer or not, but similar to all() you can use next() with a generator expression to do tasks like this instead of a list comprehension that we take the first element from. It's pretty minor, but in broader use has the advantage that it stops on the first element it finds, instead of parsing the whole list and then discarding any extra results.

[ddohler]

Just to clarify the "authenticated user" language a bit: all users in DRIVER are authenticated, in that they have a token that they use to communicate with the API, but users can have one of three roles: public, analyst, or admin. public users are created by default when a user logs in using Google SSO for the first time, and this is what I was considering to be "unauthenticated" when I wrote the task. A better formulation would have been "Only visible to users with the analyst or admin role".

An example of this logic is in the navbar controller, which provides some options (Audit Log export and duplicate record management) only to certain user roles: https://github.com/WorldBank-Transport/DRIVER/blob/master/web/app/scripts/navbar/navbar-controller.js#L116

[pcaisse]

@rmartz Good catch. I'll make a change to use the email address if available and fallback to username.

I'll also need to make a change to not allow public users to view the "Created by" field.

[pcaisse]

@rmartz Should be good to go now. To test what a public user sees, if you have Google OAuth set up, you can use that as @ddohler mentioned, or you could create a public user manually. Judging by test_views.py, I think this would do it:

pu = User.objects.create_user('public', 'public@ashlar', 'public').save()
pu.groups.add(Group.objects.get(name='public'))

Public users shouldn't have created_by come down in the JSON and shouldn't see that field in the record list.

Admins and analysts should see "Created by" the record list and it should show the email if available, falling back to the username.

[ddohler]

For reference, there is also a user-management interface available in the schema editor that allows you to create users and change their assigned permissions via the UI.

[rmartz]

I'm a bit worried that having multiple serializer classes for minor differences in response is a bit of an antipatern. Some day I'd like to see what it'd take to consolidate our ReadOnly/Write sererializers, but for this from reading online I think we can add the field dynamically in __init__():

def __init__(self, *args, **kwargs):
    super(OrderSerializer, self).__init__(*args, **kwargs)
 
    if is_admin_or_writer(self.context['request'].user):
        self.fields['created_by']  = CharField()

[pcaisse]

I'm having trouble getting this working (self.context is empty). I'm also not entirely sure I like the idea of having fewer serializers whose fields we change dynamically. Seems like it might be hard to understand and maintain.

[rmartz]

Sorry, I didn't realize that VSCode publishes PR comments immediately and edited my comment after I posted. Tested afterwards and self.context['request'] should work.

I may be overly cautious from getting burned by creating subclasses and discovering months later that their pivot factors weren't as fundamental as I originally though. My concern is that class inheritance doesn't scale the way we'd likely need for controlling the fields in a given response. Inheritance forces a linear decision tree, where the process of selecting a serializer class must follow down one pre-determined branch first, then the next, etc. It works well if each decision is a categorical distinction (Like a JSON parser class vs CSV parser class), but as the decisions become less categorical and more business logic it raises the risk that you have conflicting decision points.

Like if we want to add another level of controlling a field based on a factor orthogonal to user role (Such as a feature flag, perhaps), it isn't clear how the levels would be structured - worst case, if they are truly independent we'd need a cartesian product of serializer classes. In that case, although it's decidedly less elegant, I think having fields dynamically determined in code would be the only maintainable solution.

[rmartz]

All that said, it's not like doing class inheritance or dynamic fields for this PR specifically is an inflection point that we can't revisit later on. It's pretty squarely a Type 2 decision, so if we like class inheritance more for now then 👍

[ddohler]

Late to this party but one thing I've noticed is that DRF sometimes gets in the way of making serializers too intelligent, so the "different classes plus switching logic in get_serializer_class" is often a good way to make DRF happy even if it sometimes feels a bit strange to have a profusion of serializer subclasses.

[rmartz]

Looks good, we should break the test into smaller specific tests to make them easier to work with in the future but other than that I think we're good.

[rmartz]

We should split this out into a second test, so they can fail/pass independently of each other.

[rmartz]

We should split the username vs email behavior into different tests too... it took me a couple minutes reading the code above before I noticed this here to figure what the difference between record2_result and record3_result. Splitting them into their own tests should help make the distinction more obvious in case one fails in the future.

[rmartz]

Would it be simpler to query the record details page like '/api/records/{uuid}/?details_only=True'.format(uuid=record.uuid)? That way we only get one record back and don't have to filter the response looking for our record below.