Add link to github diff on JobRequest detail page

[Jongmassey]

fixes #4034

[sebbacon]

Excited to see this.

From the original ticket:

As part of this ticket, it would be useful to look into the feasibility of extending the comparison for JobRequests which have failed, back to the most recently successful request.

Did you consider this? IMO this would deliver the most value.

OR a feature that allows you to diff two arbitrary jobrequests rather than just sequential ones.

[Jongmassey]

Job detail screenshot

new screenshots below

[Jongmassey]

Did you consider this? IMO this would deliver the most value.
OR a feature that allows you to diff two arbitrary jobrequests rather than just sequential ones.

Yes, I did think about it but I thought I'd try the most basic implementation first just to get my head around django and job server.

I think having both "Compare to last run" and "Compare to last successful run" links wouldn't be too hard to do and could provide good value.

The arbitrary commit comparison would require more extensive UI changes.

[sebbacon]

I think having both "Compare to last run" and "Compare to last successful run" links wouldn't be too hard to do and could provide good value.

The arbitrary commit comparison would require more extensive UI changes.

Yes.

I think ideally we would add the "compare to last successful run" as part of this work, while we're thinking about it. If you choose not to, we should capture it in a new issue, as I don't want to lose it as a feature request. I can see it being super useful for tech support, and also gradually educating users that they can do the same.

[iaindillingham]

I'm requesting changes because of JobRequestManager.previous, but please do consider my other comments.

[Jongmassey]

Latest screenshots

JobRequest Detail page

No previous JobRequests for this workspace on this backend

Previous JobRequests for this workspace on this backend but none succeeded

Previous JobRequests for this workspace on this backend some of which succeeded

Job Detail

No previous runs of this action in this workspace on this backend

Previous runs of this action in this workspace on this backend but none succeeded

Previous runs of this action in this workspace on this backend some of which succeeded

[Jongmassey]

The wording of these feels clunky (esp the Job detail page) but I'm struggling to make it better. @iaindillingham and @sebbacon raised very good points that it needs to be clear on the Job detail page that this is compare the code for the whole workspace, not just the action in question.

[sebbacon]

I suspect we can help the user understand what will happen when they click partly with a little bit of UX tinkering of the sort @tomodwyer excels at?

Just spitballing but something like:

"View all repo changes since last execution on this backend"
"View all repo changes since last successful execution on this backend"

[Jongmassey]

With thanks to @tomodwyer I think most of the bikeshedding re wording is resolved. Latest screenshots updated.

[iaindillingham]

Thank you for reworking this PR. I've made several comments that I'd like you to consider; the most important relate to the use of Min and succeeded=False. I'm going to approve, because requesting changes feels heavy-handed (and I've requested changes once already). If you'd like me to spend a bit of time on JobRequestManager.previous, then please ask.

[iaindillingham]

I don't think it's a good idea to rely on JobRequest.id being monotonic. We want a temporal ordering, and JobRequest.created_at is a required field that defaults to timezone.now. Is there a reason for not using it instead?

Suggested change

	id__lt=job_request.id,
	created_at__lt=job_request.created_at,

[Jongmassey]

It's defined as an integer AutoField -

job-server/jobserver/migrations/0001_initial.py

Lines 811 to 817 in b5b1a9d

	"id",
	models.AutoField(
	auto_created=True,
	primary_key=True,
	serialize=False,
	verbose_name="ID",
	),

under what circumstances would this not be monotonic? I chose it as the sorting key as it's the PK and so should be far faster than using created_at

[iaindillingham]

Small brain snag: Why from b to a? (i.e. Why looking back in time?) Wouldn't the diff show everything that was added in a to get b in red? Clearly, this is a test: nobody will see. But this is giving me the feeling that I've missed something.

[Jongmassey]

comparing latter to previous is how my brain works for this, but clearly that's not universal?

[iaindillingham]

You're going to have to explain to me what you're doing with Min here, because I've read it, run it in the shell, and I can't shake the feeling that it's a hack.

[Jongmassey]

the values for job.status are ("failed", "pending", "running", "succeeded"). An efficient way of answering the question: "are all of the status values for all of this JobRequest's Jobs equal to 'succeeded'?" is to take the minimum (by alphabetical sort) of the Jobs' statuses. Which is to say: min("failed", "pending", "running", "succeeded") == "succeeded"

This could be made more robust by adding a CheckConstraint to that field to enforce the fact those are indeed the only permissible values or by

workspace_backend_job_requests = workspace_backend_job_requests.annotate(
                min_status=Min("jobs__status"), max_status=Max("jobs__status")            
               ).filter(min_status="succeeded", max_status="succeeded")

such that if "zzz" was added as a status then (min("succeeded","zzz")=="succeeded")&(max("succeeded","zzz")=="succeeded")==False

To use the logic in the @property of Job.status involves running the query to get all the JobRequests prior to this one for this workspace/backend, then filtering in memory for the "succeeded" ones. By using the aggregate(s) above, this filtering can be pushed into the database query.

[Jongmassey]

see 35ccbf8