The signoff view only offers a limited amount of pushes, and you can't show more of them. That's good to have, so we should do that. This is a follow up to bug 565358. This may intefere with data1.1, though.

:Pike a hint would be helpful :) Is it about selecting across multiple repositories.

The existing query works, but we'll need to split it out into an api that can load further rows, probably by push_date. I'd be scared to rely on id.

Note-to-self: #api.py:312 # if we're having a sign-off on this appversion, i.e no fallback, # show only new pushes if current_so is not None and fallback is None: pushes_q = (pushes_q .filter(push_date__gte=current_so.push.push_date) .distinct()) else: pushes_q = pushes_q.distinct()[:count] Pagination by loading AJAX that extends the table. When loading the second time, use the last push_date AND count=10 instead of the current_so's push date. E.g. def annotated_pushes(appver, loc, actions, flags, fallback, count=10, max_pub_date=None): ... if max_pub_date: # use this and :count to limit the pushes_q queryset else: # same logic as above with `if current_so ...: else: :count`

Can we make this bug depend on https://bugzilla.mozilla.org/show_bug.cgi?id=668761 The solution of this bug is going to depend on *either* incrementing the offset (e.g. `[:offset]`) or the date (e.g. `push_date__gte=some_date`) and I can see a lot of potential conflict errors. Also, since I still don't grasp #668761 (clearly, from my failure to submit a working patch) I don't feel confident about understanding how it's supposed to work on consecutive pages. I.e. is the logic going to be different on page 2?

Sure, sounds good to have that dependency on bug 668761.

A few comments on https://github.com/peterbe/elmo/commit/4fc0f1041707e33a68f6bf8657c673aa1814d340: I think the factorization is along the .annotated_pushes api. I'd factor out the flags and actions code [1] to get the range, and pass that in. The current patch computes a host of unnecessary data, we really only need the annotated pushes array. I'd also try to store the cut-off date in js code somewhere, so that we don't have to scrape the page for it. And last but not least, please don't use class or instance members for params. I know this code does that during the collection, but that's internal to the API. Having those dependencies explicit makes it much easier to test. [1] https://github.com/mozilla/elmo/blob/c67343ebc8d43e2ecc837ff98c07ff4f211505d7/apps/shipping/views/signoff.py#L138-159

(In reply to Axel Hecht [:Pike] from comment #6) > A few comments on > https://github.com/peterbe/elmo/commit/ > 4fc0f1041707e33a68f6bf8657c673aa1814d340: > > I think the factorization is along the .annotated_pushes api. I'd factor out > the flags and actions code [1] to get the range, and pass that in. > I don't know why but I've read this over and over and I don't understand what you mean. Would you mind trying to elaborate or explain it in some other way. Can you demonstrate what you mean? And how does that relate to what I accomplished so far in the branch? Was I getting the wrong data out? > The current patch computes a host of unnecessary data, we really only need > the annotated pushes array. > Sure. I can review that later once it works. > I'd also try to store the cut-off date in js code somewhere, so that we > don't have to scrape the page for it. > I'm not sure I'm convinced JS is any better than data attributes. Also, this way it makes it easy to make sure we only use one loop. > And last but not least, please don't use class or instance members for > params. I know this code does that during the collection, but that's > internal to the API. Having those dependencies explicit makes it much easier > to test. > I think I know what you mean. Sure thing. I'll clean that up once it works. What I need help with is the data query piece. The code is horrible to work with and it's obtuse and complicated. I understand that a lot of that is because of optimization and time constraints. I need some help through the jungle at the moment.

I've updated https://github.com/peterbe/elmo/tree/feature/bug-658656-implement-more-pushes-on-signoff-view with something that's hopefully answering all your comments above. Basically the refactoring we talked about on the phone. And to avoid the data attribute sifting I changed it so that the AJAX does a JSON thing instead which contains the new HTML. By doing that I can include what the next `min_push_date` is and how many pushes there are left. And we need how many pushes there are left to be able to remove the "Load more" link when there's nothing left to load. PS. Note that I made the default count (aka. limit) to 5 just for testing purposes.

Yep, that's basically doing what I had in mind, but a few comments: The pushes_left stuff doesn't seem to do what it's supposed to, and I don't think we need it. I'd only trigger that code path if there's a more-data query coming back with less rows, and optimize the thing away. The other way to implement it would be to, at the point where you have min_push_date, get back to the original pushes_over_time pushes_q and do a .filter(push_date__lt=min_push_date).exists(). I'd probably disable the more-pushes UI and not remove it, too. The attach_diffbases seems to be a hack. I wonder if we should move that into a class-wide flag instead. I'd also move the check to 277, where it modifies the sod['diffbases']. I'd move the min_push_date out of the for loop, and use min_push_date = pushes[-1]['push_date'] if pushes else None. I personally prefer to have things that can get values in two branches of the code to be defined before the code path branches, initial_diffs = [] is such a case. I guess you can get rid of the changes to signoff-rows.html now that we're having the min_push_date in code?

(In reply to Axel Hecht [:Pike] from comment #9) > Yep, that's basically doing what I had in mind, but a few comments: > > The pushes_left stuff doesn't seem to do what it's supposed to, and I don't > think we need it. I'd only trigger that code path if there's a more-data > query coming back with less rows, and optimize the thing away. > > The other way to implement it would be to, at the point where you have > min_push_date, get back to the original pushes_over_time pushes_q and do a > .filter(push_date__lt=min_push_date).exists(). > I'm not convinced. Perhaps there's something obvious I'm not seeing. By doing `pushes_left` as a count (an integer) we can have that nice UI that says "_More pushes_ (13 left)" which, IMHO, is very user-friendly. Alternative 1 is to compare `self.count` with `len(pushes)` and simply do `pushes_left = self.count == len(pushes)`. But what if there are exactly 10 pushes (and self.count being 10) then you'd be lying to say that there are more pushes. The count avoids that. Alternative 2 basically involves another query too. But instead of `select count()` it's an exists so it's virtually the same in terms of processing. Except, the count gives something we can actually use the UI. > I'd probably disable the more-pushes UI and not remove it, too. > Yeah, removing it isn't ideal but I'm not really sure what "disable" means for a actionable link. Do you mean to do something like `$('a.more').click(function() {return false})`? Or do you mean to make it not a link and just the text "More pushes"? > The attach_diffbases seems to be a hack. I wonder if we should move that > into a class-wide flag instead. I'd also move the check to 277, where it > modifies the sod['diffbases']. > I like that idea! I add it a class attribute and in annotate_pushes() it now refers to `self.attach_diffbases`. > I'd move the min_push_date out of the for loop, and use > > min_push_date = pushes[-1]['push_date'] if pushes else None. > Good idea! I changed it. > I personally prefer to have things that can get values in two branches of > the code to be defined before the code path branches, initial_diffs = [] is > such a case. > Done. Good idea. > I guess you can get rid of the changes to signoff-rows.html now that we're > having the min_push_date in code? Gone. PS. I updated my branch/fork with most of the changes from above.

(In reply to Axel Hecht [:Pike] from comment #9)

Comment on attachment 8343074 [details] [diff] [review] bug658656.diff Review of attachment 8343074 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the lag, I had too much travel in the past two weeks. I found a few details, but most importantly, reading this patch again, I think our choice of min_push_date is really unfortunate and confusing. At least at the point where we query push_date__lt=min_push_date, the name doesn't make sense. I think I like next_push_date better. Not great either. Anything that indicates that it's the limit between the two queries. push_query_separation_date is long. r-, as I think I'd like to look at the patch again. ::: apps/shipping/static/shipping/js/signoffs.js @@ +137,5 @@ > + }); > + > + // Firefox has a tendency to "cache" that a button should remain disabled > + // even if you refresh the page without a force-refresh > + $('button.load-more[disabled=""]').removeProp('disabled') should this be removeAttr instead? ::: apps/shipping/tests/test_signoff.py @@ +460,5 @@ > self.locale, > self.av, > + actions=actions, > + flags=flags, > + fallback=None, fallback=None is default, I'd suggest to make use of that. @@ +475,5 @@ > self.locale, > self.av, > + actions=actions, > + flags=flags, > + fallback=None, here, too. If not using the default, I would suggest to define fallback=None, and reuse that. @@ +505,5 @@ > self.locale, > self.av, > + actions=actions, > + flags=flags, > + fallback=None, dito @@ +552,5 @@ > self.locale, > self.av, > + actions=actions, > + flags=flags, > + fallback=None, just enumerating them where I see them. @@ +610,5 @@ > self.locale, > self.av, > + actions=actions, > + flags=flags, > + fallback=None, . @@ +655,5 @@ > self.locale, > self.av, > + actions=actions, > + flags=flags, > + fallback=None, . @@ +688,5 @@ > self.locale, > self.av, > + actions=actions, > + flags=flags, > + fallback=None, . @@ +707,5 @@ > + self.locale, > + self.av, > + actions=actions, > + flags=flags, > + fallback=None, . @@ +726,5 @@ > + self.locale, > + self.av, > + actions=actions, > + flags=flags, > + fallback=None, . @@ +742,5 @@ > + self.locale, > + self.av, > + actions=actions, > + flags=flags, > + fallback=None, . ::: apps/shipping/views/signoff.py @@ +115,5 @@ > + def annotated_pushes(self, > + lang, appver, > + min_push_date=None, > + actions=None, flags=None, fallback=None, > + count=10): Make this fallback to self.count, too? @@ +159,5 @@ > initial_diff = [] > > + if min_push_date: > + # Indendepent of action cutoffs we just, filter by the > + # minimum push date and the limit English grammar. How about: We're asked for pushes beyond the original cutoffs, just use min_push_date and the limit. @@ +171,5 @@ > + # This is the first load whereby we want to select all pushes up > + # to a certain cutoff. > + cutoff_dates = [] > + action4id = dict((a.id, a) for a in actions) > + initial_diff = [] double definition

Addresses all nits. Also, I think `next_push_date` is a good variable name.

Comment on attachment 8357464 [details] [diff] [review] bug658656-2.diff Review of attachment 8357464 [details] [diff] [review]: ----------------------------------------------------------------- Wheee, r=me, thanks. One nit, I found a min_push_date, that you copied from my previous bug comment. ::: apps/shipping/views/signoff.py @@ +161,5 @@ > initial_diff = [] > > + if next_push_date: > + # We're asked for pushes beyond the original cutoffs, > + # just use min_push_date and the limit. Found one leftover :-) In all fairness, that's from my bug comment, but still.

Commit pushed to develop at https://github.com/mozilla/elmo https://github.com/mozilla/elmo/commit/8abcc8f8430db52350a3dbce950efecd3ce68771 bug 658656 - Implement "More pushes" on signoff view, r=Pike

I've tested this locally, and found a bug, which made the button not show at all. The pushes_left call was triggered after reducing the query, which made the count always turn out 0. I also made sure that they all run distinct() before, as the counts were off.

Comment on attachment 8371592 [details] [diff] [review] follow-up Review of attachment 8371592 [details] [diff] [review]: ----------------------------------------------------------------- Tests still pass? How come they didn't catch this? I'm r+ but would like to see the nit about the comment addressed. ::: apps/shipping/views/signoff.py @@ +172,5 @@ > else: > # This is the first load whereby we want to select all pushes up > # to a certain cutoff. > + # All pushes in our query count > + pushes_left = pushes_q.distinct().count() It's really picky but the comment "This is the first load whereby..." used to be right above the `cutoff_dates = []` line. Now that this new comment and line squeezes itself in there, that original comment is now very far away from the code it used to document.

Commit pushed to develop at https://github.com/mozilla/elmo https://github.com/mozilla/elmo/commit/c5d95c03c6880b23854479b264e0eb7833799d07 bug 658656, fix pushes_left, r=peterbe

Commit pushed to develop at https://github.com/mozilla/elmo https://github.com/mozilla/elmo/commit/ed3330a8893f426800b51e3ddc8095ae1806b766 bug 658656, fix comments

The reason why this tests didn't catch this is that we only tested len(pushes) mostly, but only one code path tested pushes_left. I added that to the tests. Locally by doing the fix and the tests in two commits, then rebased-swapped them, and verified that the tests actually failed before, and now pass. Squashed that for landing. I forgot the comment fix, so that's the second landing. I moved the comments explaining the variables to post-line comments, so the code-block this-is-what-we-do-here stands out as it used to.