660775 - [shipping] signoff2 etag view decorator lacks test coverage

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P3)

Description

[Peter Bengtsson [:peterbe]]

The `signoff` view in shipping has an ETag decorator which depends on last actions and the user permissions. This ETag prevents a user from seeing stale signoffs which is great because it also prevents excessive loading of the signoff view potentially (the whole idea of ETags :).

However, it lacks test coverage.

Comment 1

[Peter Bengtsson [:peterbe]]

Attachment: Adds test coverage for the etag_signoff decorator

This ups the coverage from 17% to 52% but the added 35% is just blind "luck" since my test does not test the output of the 'signoff()' view.

Comment 2

[Axel Hecht [:Pike]]

Comment on attachment 536297 [details] [diff] [review]
Adds test coverage for the etag_signoff decorator

This has a tentative r=me, given that we still need to iron out how to fix the original bug. Not sure if we should land this yet.

Comment 3

[Peter Bengtsson [:peterbe]]

As far as the scope in question, I think this test is complete. 

The problem lies in a step removed from this particular bug. This patch sets out to fix the stated bug and it has done so. "signoff2 etag view decorator lacks test coverage"

That problem is to do with why the etag decorator doesn't work. Or rather, why it doesn't work when behind apache. (Remember I'm unable to reproduce the problem when I run my elmo site behind Nginx)

I'd like to land it so I can get rid of the branch but will await your final approval.

Comment 4

[Axel Hecht [:Pike]]

The argument against landing the tests is that if our setup with etags doesn't work, we'd back it out. Let's fix bug 659108 for real first.

Comment 5

[Peter Bengtsson [:peterbe]]

bug 659108 is now fixed. Can we look at landing my patch for the trivial tests now?

Comment 6

[Axel Hecht [:Pike]]

Want to update the test to incorporate the new paramaters like Run and Push, and that Cache-Control should be private?

Comment 7

[Peter Bengtsson [:peterbe]]

Attachment: tests all scenarios of data changes relevant for the etag_signoff cache function

this test covers all cases I think. It just checks that the etag identifier changes as the underlying circumstances changes accordingly.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 551362 [details] [diff] [review]
tests all scenarios of data changes relevant for the etag_signoff cache function

Review of attachment 551362 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with two nits:

The tmp repo and locale could just as well be the Polish 'pl' repo and locale, and you wouldn't have the somewhat off-custom naming. See below.

Also, the test would be just as strong if we just check for different etags, and drop the checks on particular elements. That'd have the advantage that the test would be relevant if we'd decide to change how we compose the etag value, like, if we'd decide to use a hash of the tuple instead of the tuple or something like that.

::: apps/shipping/tests.py
@@ +619,5 @@
> +          forest=last_push.repository.forest,
> +          name=last_push.repository.name + '...',
> +          url=last_push.repository.url,
> +          locale=_tmp_locale,  # the difference
> +        )

The naming here is funky, mind just using the existing 'pl' repo and locale that are already in the fixture? As you're already relying on the 'de' one, that's not any worse.

Comment 9

[Peter Bengtsson [:peterbe]]

Landed and no more of those "_tmp..." variable names. 

https://github.com/mozilla/elmo/commit/89032d5312f9ca709a65a399e736a941e1d490c1

Comment 10

[Axel Hecht [:Pike]]

Reopening, sorry. I'd really like to get the comments addressed:

The repo for polish exists in the fixture, so instead of creating a new one, you should simply do
other_repo = Repository.objects.get(locale=other_locale)

Also, the various checks on the actual etag string parts are still in there.

Comment 11

[Peter Bengtsson [:peterbe]]

Attachment: etag tests don't check the content of the identifier

Not obsoleting the old patch since that was landed.

Comment 12

[Peter Bengtsson [:peterbe]]

Landed second patch https://github.com/mozilla/elmo/commit/5fedb6d72575a76758aa01d963c5bdb72974f92d

Comment 13

[Peter Bengtsson [:peterbe]]

Changing status