1308990 - Display last modified author and timestamp in exclusion Admin

Status: RESOLVED WONTFIX

(Tree Management :: Treeherder, defect)

Description

[Cameron Dawson [:camd]]

There are a few parts to this bug:

1. The "author" field of ``exclusion_profile`` and ``job_exclusions`` only gets populated on creation.  It should be updated to the new user on update, but it is not.

2. On ``exclusion_profile`` we store the author_id and the last modified date.  But on ``job_exclusions`` we only store the author.  We should add the modified date field to ``job_exclusions`` too.

3. We should display the last modified and the author in the UI so Sheriffs can see who made the last changes.

Comment 1

[Cameron Dawson [:camd]]

Here are some parts of the code related to this:

https://github.com/mozilla/treeherder/blob/fb435816b06ba4a77a897a82a049de3747cb2cc8/ui/js/controllers/admin/exclusions_detail.js#L52-L52

https://github.com/mozilla/treeherder/blob/4c8d3f69e67e5608c6241fe0bca69a78af150fe6/ui/js/models/job_exclusion.js#L70-L95

https://github.com/mozilla/treeherder/blob/fb435816b06ba4a77a897a82a049de3747cb2cc8/ui/partials/admin/exclusions_list.html#L1-L1

Comment 2

[Kris Rivera]

Attachment: Proposed Fix

This is what I currently have for my proposed fix for this issue. I have a couple of questions that need to be answered to be able to finish fixing this completely.

1) I don't know how to add a database field correctly and persist the field to my mysql db.
2) Right now, I have the id of the person who created/updated the exclusion profile or job exclusion being persisted (and displayed), but I would think that the user name or email address of that user would be better than seeing the user.id. How can I persist this value? I assume that I can create another db field (author_name) and store request.user.username there, but I can't do that without knowing how to do #1 above.
3) What date format should we be using? I currently have MM/dd/YYYY

Comment 3

[Cameron Dawson [:camd]]

(In reply to Kris Rivera from comment #2)
> Created attachment 8803721 [details]
> Proposed Fix
> 
> This is what I currently have for my proposed fix for this issue. I have a
> couple of questions that need to be answered to be able to finish fixing
> this completely.
> 
> 1) I don't know how to add a database field correctly and persist the field
> to my mysql db.

This is done with a migration.  You add the field to the appropriate model in models.py, and then run ``./manage.py makemigrations.  See the docs here for how this works: https://docs.djangoproject.com/en/1.10/ref/django-admin/#django-admin-makemigrations

> 2) Right now, I have the id of the person who created/updated the exclusion
> profile or job exclusion being persisted (and displayed), but I would think
> that the user name or email address of that user would be better than seeing
> the user.id. How can I persist this value? I assume that I can create
> another db field (author_name) and store request.user.username there, but I
> can't do that without knowing how to do #1 above.

You can get the author email field through a reference, since it's a ForeignKey.  But you'll need to do that on the python side.  You'd add that in our Django Rest Framework Viewset called JobExclusionViewSet and ExclusionProfilViewSet.  Those handle that apis that the UI calls.  Right now we don't override the default for getting the list of either.  I believe you'd need to override it to return the author.email.  See: http://www.django-rest-framework.org/api-guide/viewsets/  You may end up needing a Serializer for that, too.

> 3) What date format should we be using? I currently have MM/dd/YYYY

I'd go ahead and use the same we do on the main page: Mon Oct 24, 12:22:53.  We have lots of international folks so the month/day can be swapped.  This way it's clear.  But you could probably skip the "Mon" part, if you like.  Depends if you're constrained on the width.

Comment 4

[Cameron Dawson [:camd]]

Comment on attachment 8803721 [details]
Proposed Fix

I think you're on the right track here.  :)

Comment 5

[Kris Rivera]

Attachment: Review Pull Request

Cameron, I made the necessary changes and was able to test them out successfully with your comments. Thank you

Comment 6

[Cameron Dawson [:camd]]

Comment on attachment 8804089 [details] [review]
Review Pull Request

Hi Kris--  I've suggested a few changes.  Would you take a look and see if this approach would work?  We don't need to add the author email to the django model for ``JobExclusion`` and ``ExclusionProfile``.

Thanks!!  Please just change this back to ? with my email when you're ready for another review.

Comment 7

[Cameron Dawson [:camd]]

Comment on attachment 8804089 [details] [review]
Review Pull Request

Hi Kris-- Clearing the review flag for now.  I tried your branch, but get the error I mentioned in the PR.  Would you try that out and see what you get?  I'll be happy to take another look as soon as that's resolved.

Thanks!

Comment 8

[Ed Morley [:emorley]]

We're going to replace exclusion profiles with people always using the in-tree (mozilla-central) tier values instead (bug 1387640).