Closed
Bug 659108
Opened 13 years ago
Closed 13 years ago
signoff2 view caches too good
Categories
(Webtools Graveyard :: Elmo, defect, P1)
Webtools Graveyard
Elmo
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Pike, Assigned: Pike)
References
Details
Attachments
(2 files)
(deleted),
patch
|
stas
:
review+
peterbe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
peterbe
:
review+
|
Details | Diff | Splinter Review |
The signoff2 view cashes too good, I often see the same pending sign-offs after I accept one.
We may want to use the latest action as key to a ETag?
Comment 1•13 years ago
|
||
What are the steps to reproduce it?
Assignee | ||
Comment 2•13 years ago
|
||
You'll need to sign off and/or review. Can you get an updated snapshot on -dev- ? Then I can grant you priviledges on that version.
Updated•13 years ago
|
Priority: -- → P1
Updated•13 years ago
|
Assignee: nobody → peterbe
Target Milestone: --- → 1.2
Assignee | ||
Comment 3•13 years ago
|
||
Stealing this bug from Peter.
I had to cache not just on latest action, but also on the permissions we use as otherwise the page doesn't update the UX. That is, the vary on cookie apparently get's overruled by the ETag.
Given that this is real webdev, I'd like to get Peter's review on this. Also, stas for business logic here.
Assignee: peterbe → l10n
Status: NEW → ASSIGNED
Attachment #535339 -
Flags: review?(stas)
Attachment #535339 -
Flags: review?(peterbe)
Comment 4•13 years ago
|
||
Comment on attachment 535339 [details] [diff] [review]
etag on permissions and latest action
Review of attachment 535339 [details] [diff] [review]:
-----------------------------------------------------------------
::: apps/shipping/views/signoff.py
@@ +104,5 @@
>
> +
> +def etag_signoff(request, locale_code, app_code):
> + actions = Action.objects.filter(signoff__locale__code=locale_code,
> + signoff__appversion__code=app_code).order_by('-pk')
How about:
try:
_id = Action.objects.filter(...).latest('when').id
except DoesNotExist:
_id = "no signoff"
You could also add
class Meta:
get_latest_by = 'when'
to shipping.Action, and then use latest() without any args.
Attachment #535339 -
Flags: review?(stas) → review+
Comment 5•13 years ago
|
||
Comment on attachment 535339 [details] [diff] [review]
etag on permissions and latest action
Review of attachment 535339 [details] [diff] [review]:
-----------------------------------------------------------------
::: apps/shipping/views/signoff.py
@@ +109,5 @@
> + can_signoff = request.user.has_perm('shipping.add_signoff')
> + review_signoff = request.user.has_perm('shipping.review_signoff')
> + try:
> + _id = str(actions.values_list('id',flat=True)[0])
> + except:
Why a catch-all? I'd suspect the only acceptable exception we can expect is IndexError. So it should be:
except IndexError:
Attachment #535339 -
Flags: review?(peterbe) → review+
Assignee | ||
Comment 6•13 years ago
|
||
https://github.com/mozilla/elmo/commit/32d19258fbe3f216bb4818bc80b1aab386ebc88b, FIXED.
Technically and network wise, this was done as a hotfix. I fail to deploy on -dev-, though, mostly because I can't recover from the env change without stas' brain. As that deployment is failing, I'm not deploying on -stage- either. So it's not all that hot.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•13 years ago
|
||
This doesn't really work in practice.
Shyam, any idea why we send out ETags on bm-l10n-dashboard01 that don't make it to the client? Example URL would be https://l10n-stage-sj.mozilla.org/shipping/signoffs/is/fx-aurora. Once I reload it's updated, but just surfing to the page get's an old ETag response.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 8•13 years ago
|
||
I see "X-Cache-Info" headers in the responses, that sounds like zeus is talking to us, http://www.zeus.com/community/articles/making-most-content-caching.
I verified locally that on a revisit to a signoff page, we're not getting a hit to confirm the etag.
Probably we need to make Cache-Control: no-cache ? Stabbing in the dark here, http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9 doesn't make me really understand what we should do. In particular the cache verification stuff with max-age:0. Zeus says "no-cache", and the http docs read like it's the right way to enforce revalidation every time.
Assignee | ||
Updated•13 years ago
|
Target Milestone: 1.2 → ---
Assignee | ||
Comment 10•13 years ago
|
||
working on an update here.
Assignee | ||
Updated•13 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 11•13 years ago
|
||
This should fix Zeus caching our responses, and also fixes a few missing datapoints on the etag. Also, with comments now.
I only checked the fragment I touched for pep-iness, the rest of the file is still dirty as harry.
Attachment #549079 -
Flags: review?(peterbe)
Comment 12•13 years ago
|
||
Comment on attachment 549079 [details] [diff] [review]
make cache-control private, and add pushes and runs to etag
Review of attachment 549079 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #549079 -
Flags: review?(peterbe) → review+
Assignee | ||
Comment 13•13 years ago
|
||
https://github.com/mozilla/elmo/commit/48d548ab4f8482bc0e1fd7732b775955bb143273, marking FIXED again.
I updated -dev- and verified that it does what I expect, at least while reviewing a sign-off, which does reload, and when going the roundtrip without a mod, which get's the not-modified.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Product: Webtools → Webtools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•