It would be nice if we could know who was classifying performance alerts, especially if we're going to implement some kind of "autoclassification" for them (see bug 1285322). I think we can do this by adding a "classifier" field to the PerformanceAlert model, referencing the user's table (see the jobnote model for an example of this: https://github.com/mozilla/treeherder/blob/bfc5076/treeherder/model/models.py#L729). Then when the user initiates a modification request on the performance alert, we can update this field (you will need to add some functionality to the PerformancAlert viewset for this: https://github.com/mozilla/treeherder/blob/bfc5076/treeherder/webapp/api/performance_data.py#L241). On a UI level, perhaps we could show the classifier when the user hovers over the alert text using a tooltip.
:camd told me that there could be a mailing list I can mail so that people know a new field is being added and the migration can go smoother, but we aren't sure which one to mail to. Anyway, here are the steps I took, and please let me know if I'm missing something! * Create a new field ``classifier`` * Create a new method ``update`` for PerformanceAlertViewSet, so that if ``related_summary_id`` is included in POST data, classifier can automatically be identified * Create a new field ``ClassifierField``, since we don't have an endpoint for retrieving user data. I thought this would be the best way to to take a values that have different input/output format * Add uib-tooltip to display classifier e-mail.
Comment on attachment 8773852 [details] [review] [treeherder] crosscent:Bug1288530 > mozilla:master I've changed the custom field to SlugRelatedField.
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/5da09daa72121d031e51ddfa4adca99d838eb4ac Bug 1288530 - Add a "classifier" field to PerformanceAlert, referencing the user model (#1718)
Hey, so I merged this (see above) and it seems to work well on stage. However, after playing with it a bit I think it would be better if we actually set the classification on *any change*, not just those based on reassigning/downstreaming the alert. Correspondingly, we would show the tooltip on the text of the alert itself (not the reassigned/downstream status span). Note that in the case that the alert state is "untriaged", we should not claim it is autoclassified (though we should show the user information if there is any). Roy, would you be interested in doing an updated PR for this? Sorry for not catching this earlier.
I can definitely provide an updated PR for this. I'm not very familiar with modifying Alerts, so could you elaborate on what other changes are?
I'll comment on the old PR, the changes should be quite straightforward. :)
Hey Will, thanks for pointing out all the changes to make the new PR. What I meant was what other changes were possible when modifying an Alert, but you've already answered me on IRC! I forgot about "acknowledged." Anyway, I have attached a new PR, and this is my first time creating a new PR on top of a previous commit, so please let me know if the commit isn't correct. I am also wondering if the nested ternary condition should be refactored out to a separate function. Thanks for all your help!
Comment on attachment 8773852 [details] [review] [treeherder] crosscent:Bug1288530 > mozilla:master You requested the review on the wrong PR. :) Also, please rebase the new one against master.
Comment on attachment 8775347 [details] [review] [treeherder] crosscent:Bug1288530 > mozilla:master Thanks for the detailed commands! I think I'm getting a little better with Git everyday now. I hope I got the correct PR for review this time.
Attachment #8775347 - Flags: review?(wlachance)
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/ef46fa00eb0919e92f11ffdfe6a6926541ab409c Bug 1288530 - Update classifier unconditionally, and move classifier tooltip in AlertView (#1742)
Comment on attachment 8775347 [details] [review] [treeherder] crosscent:Bug1288530 > mozilla:master Looks great, merged.
Attachment #8775347 - Flags: review?(wlachance) → review+
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.