Bug 1121998 - Add the ability to retrigger all pinned jobs

[vaibhavmagarwal]

[vaibhavmagarwal]

I need to call selectJob() function to set the $scope.job and it has to be sequential to send correct job_ids to retriggerJob(). I know it is a bit hacky, so let me know if there is a better way.

[tojon]

@vaibhavmagarwal would you like to be assigned this bug?

[vaibhavmagarwal]

Ok.

[vaibhavmagarwal]

I need to call selectJob() function to set the $scope.job and it has to be sequential to send correct job_ids to retriggerJob(). I know it is a bit hacky, so let me know if there is a better way.

[edmorley]

Maybe @camd has some suggestions? :-)

[maurodoglio]

I think the best way to deal with this is to convert the retrigger endpoint to a list_route and pass it a list of jobs. I prefer a single job being a special case rather than make potentially >1000 requests to the api as a consequence of a click.

[edmorley]

Thank you for working on this! :-)
I wonder if we need to surface this in the UI more obviously than as an option on the dropdown menu?
Perhaps its own button on the panel? @tojonmz what do you think?

[vaibhavmagarwal]

@edmorley we want the "retrigger all" button to not be easily discoverable, since users can misuse it. So I have placed it in a dropdown menu. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1121998#c3

[edmorley]

@edmorley we want the "retrigger all" button to not be easily discoverable, since users can misuse it. So I have placed it in a dropdown menu. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1121998#c3

Ah ok :-)

[maurodoglio]

I'm not sure I understand the rationale to have the "retrigger all" accessible to everyone but not easily discoverable. Making things not easily discoverable on purpose sounds like a ux anti-pattern to me. We should probably come up with a policy (requests throttling maybe?) and adopt it.

[tojon]

Given we're putting a warning before proceeding we could put it somewhere else in the pinboard (perhaps as a full button underneath the present 'Save'), but iirc @rvandermeulen had indicated a dropdown would be fine for accessibility. On the flip side we have 'Clear All' in that dropdown, and if the user misses their selection to retrigger all they would destructively clear their pinboard. :) Ryan/we could test out the current approach on stage and make it a full button if needed.

[vaibhavmagarwal]

I could send this function either list of jobs or list of job_ids. I think sending it a list of jobs and then extracting whatever information we want is better. Let me know if I should just send it a list of job_ids.

[vaibhavmagarwal]

@maurodoglio I have made the changes suggested by you. However, I am getting errors in travis for a test: https://travis-ci.org/mozilla/treeherder/builds/74163454
I have checked out http://www.django-rest-framework.org/api-guide/testing/ but I am not understanding what is going wrong. Do you have any idea what I am missing? Thanks.

[maurodoglio]

The error you are getting is a side effect of how QueryDict objects work in django. In the job retrigger endpoint you are retrieving the list of job ids from request.data, which retrieves only the last item if the value is a list. In your case the value returned was the string '10' and when you iterate over it you get a '1' and a '0'. The id of the jobs you are comparing in your test are 10 (expected) and 1 (actual).
To obtain a list of values from a QueryDict you need to call its getlist() method (see the QueryDict documentation).

[vaibhavmagarwal]

@maurodoglio I noticed there is a dissimilarity between post in Django and post from AngularJS.
request.data sent from AngularJS directly gets converted to a dictionary in Django and I can't use getlist:

  File "/home/vagrant/venv/local/lib/python2.7/site-packages/rest_framework/views.py", line 453, in dispatch
    response = handler(request, *args, **kwargs)
  File "/home/vagrant/treeherder/treeherder/webapp/api/utils.py", line 200, in use_jobs_model
    return model_func(*args, jm=jm, **kwargs)
  File "/home/vagrant/treeherder/treeherder/webapp/api/jobs.py", line 145, in retrigger
    job_id_list = request.data.getlist("job_id_list")
AttributeError: 'dict' object has no attribute 'getlist'

Whereas post from Django ( in test_jobs_api.py ) is a QueryDict object, so I have to use a getlist() method there.

[vaibhavmagarwal]

r? @maurodoglio

[maurodoglio]

I just found out that request.data is a QueryDict when the content type of the request is multipart form, otherwise it's a normal dict (see the docs). In the test that was failing you are using the APIClient, which by default set the content type to multipart form. In theory the treeherder api is setup to only parse json content type, but there must be something wrong there. Anyway, you can expect request.data to be a dict if you set the request format to json in your test (see the docs for reference)

[vaibhavmagarwal]

@maurodoglio fixed. Thanks for the docs references!