Right now, we cut off the initial set of pushes shown in the signoff view by - 10 pushes - last accepted sign-off This is stupid, as it doesn't show a pending sign-off that is 11 or 12 pushes old. Moreso, we have a summary section that lists all kinds of sign-off artifacts (pending, approved, rejected), and we should show as many pushes as it takes to make that summary not depend on the cut-off. This does boil down to probably making getlist return more than just the flaglist, as that's what we care about. May be data1.1 dependent, yuck.

Is this a dupe of bug 658656 or does it depend on it? not clear.

Nope, the two are different. This bug is about showing all changesets that are in the summary by default. Loading more data with bug 658656 is just that, loading more data.

Marking this to depend on rowcollector refactor bug 761662, as half the involved code moves then.

Note-to-self: if there is no current sign-off and no fallback sign-off, you might get 10 pushes hiding the 11th push which has a pending sign-off. See api.py: 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] # <-- THIS IS BAD! It's bad in the sense that the table of pushes doesn't reflect what you expect from reading the executive summary one-liner at the top. In the else, case instead of just picking the last 10 (or whatever), search for the last pending || rejected sign-off as the date limiter. Being able to load 10 more (pagination, see #658656) makes it possible to get the last pending||rejected by manually clicking "see more old pushes" (or whatever it's phrased as).

(In reply to Peter Bengtsson [:peterbe] from comment #4) > Note-to-self: if there is no current sign-off and no fallback sign-off, you > might get 10 pushes hiding the 11th push which has a pending sign-off. The "not" here needs to work a tad different. "not ( current_so is not None and fallback is None)" boils down to not current sign-off or fallback.

I took a glance at https://github.com/peterbe/elmo/compare/mozilla:develop...peterbe:feature/bug668761-all-sign-offs-in-sign-off-summary, and I think this more complicated than necessary. I suggest to move from a sign-off to a date, and to move the fallback logic into the ACCEPTED section, like if Action.ACCEPTED in flags: a = action4id[flags[Action.ACCEPTED]] current_so = a.signoff initial_diff.append(a.signoff_id) changes to if Action.ACCEPTED in flags: a = action4id[flags[Action.ACCEPTED]] if fallback is None: cutoff_date = a.signoff.push.push_date etc... initial_diff.append(a.signoff_id)

I also removed all the junk debugging print lines so I'm pretty sure it's ready for review. I unfortunately only managed to set aside time to this on a Friday afternoon so there could be more things to consider. For example, perhaps the unit test isn't comprehensive enough.

Comment on attachment 720169 [details] [diff] [review] fixes from feedback Review of attachment 720169 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this will work, but the idea is right. Details inline For the test, I'd go in and not bother about that many pushes, 5 or so should be good enough. You can pass in count as needed. I'd try something like -no-op push -accepted push (cancel later, or remove) -pending push (can reject later) -pending push -dummy push I'd test, all with count=1: 5 pushes, no sign-offs. -> 5 pushes 1 accepted signoff on push 2, let's call that a 2a. -> 4 pushes 2a, 3p (pending signoff shouldn't cancel accepted) 2a, 3r 2a, 3r, 4p 2c, 3r, 4p (this should hide push 1 and 2 from annotated_pushes) -> 3 pushes ::: apps/shipping/views/signoff.py @@ +135,4 @@ > initial_diff.append(a.signoff_id) > if Action.PENDING in flags: > + a = action4id[flags[Action.PENDING]] > + cutoff_date = a.signoff.push.push_date I think this needs to be taking the min date, as below. Right now, a scenario of - accepted push - new pending push will drop the accepted push from the cutoff_date. @@ +139,4 @@ > initial_diff.append(action4id[flags[Action.PENDING]].signoff_id) > if Action.REJECTED in flags and len(initial_diff) < 2: > + a = action4id[flags[Action.REJECTED]] > + cutoff_date = a.signoff.push.push_date .. same here.

I've read your comment three times and I just don't understand what it is you're saying. Would you perhaps like to take over this bug? After all you wrote all that magic to do with `forest4times`, `treename4forest`, `for (_s, _e, _t, _tc, _f) in (` etc. so it'd be a walk in the park for you. Or perhaps we can dedicate a Monday meeting to going over it. I'm not making any progress here and I could work on something else instead.

I wrote all the unit tests first and then started refactoring it to avoid any excess.

Comment on attachment 734874 [details] [diff] [review] fixes from review Review of attachment 734874 [details] [diff] [review]: ----------------------------------------------------------------- The actual code looks fine, but the tests are funky. I'm not really happy with the mix if fixtures, deleted data of those, and then generating different but equal fixture data for each test. The fallback test was just wrong, also, I'm not exactly sure what it tests. And the cancelled sign-off was just for the sake of writing, it doesn't make sense to recreate. I'd not test fallback at all, unless you want to start doing AVTs over time, which would test a different part of the code I guess. If you're going for individual tests for each case, you could even generate them programmatically: 5 pushes, older on the left, newer on the right: 3 2 1 P P P P P Test this for count=1, 10. For P1, test accepted, pending, rejected. For each of those, test P2 as accepted, pending, rejected. For each of those, test P3 as accepted, pending, rejected. That'd be 2 + 3*(1+3*(1+3)) = 41 tests, but it'd be bullet proof. You should be able to generate those automatically, too, I guess. But maybe that's food for bug 761657, test data1.1. At the very least, we should factor the test data right here, and only create data that impacts the code path we're testing. ::: apps/shipping/tests/test_signoff.py @@ +354,5 @@ > eq_(revisions, [u'l10n de 0003', u'l10n de 0002']) > + > + > +class SignOffAnnotatedPushesTest(TestCase): > + fixtures = ["test_repos.json", "test_pushes.json", "signoffs.json"] What of those fixtures remains after you delete the pushes? Should we refactor those? @@ +428,5 @@ > + count=3 > + ) > + eq_(len(pushes), 3) > + > + def test_5_pushes_with_fallback(self): I don't think testing fallback and just a pending sign-off together provides much value. Not sure what this test is testing for realz. @@ +459,5 @@ > + signoff=signoff0, > + flag=Action.PENDING, > + author=self.peter, > + ) > + assert fallback.flag == Action.PENDING, fallback.flag fallback is a Push, not an Action. @@ +468,5 @@ > + flags, > + fallback, > + self.locale, > + self.av > + ) I'd feel better if you made count explicit here. @@ +736,5 @@ > + Action.objects.create( > + signoff=signoff, > + flag=Action.CANCELED, > + author=self.peter, > + ) This sign-off only made sense in my consecutive description of one test, as you're generating the data from scratch each time, there's no need to create it.

Greatly reduced repetition on the tests

Comment on attachment 737659 [details] [diff] [review] Improvements from past review Review of attachment 737659 [details] [diff] [review]: ----------------------------------------------------------------- r=me with two nits. Revert the changes to signoffs.json? They're not doing anything in this patch. Also, strip the 2nd cancelled signoff from the last test? I had that initially in my test layout, but that was just there to incrementally build the data and the tests. Now that you're rebuilding the data from scratch, that doesn't carry any value. (Also, the comment leading up to it was wrong, but removing that is the right option). ::: apps/shipping/fixtures/signoffs.json @@ +207,3 @@ > "end": null > } > } Revert this? ::: apps/shipping/tests/test_signoff.py @@ +589,5 @@ > + # the last (aka. first) one should have a signoff with an > + # action on that is accepting > + eq_(pushes[-1]['signoffs'][0]['action'].flag, Action.ACCEPTED) > + > + def test_5_pushes_2nd_cancelled_3rd_rejected_4th_pending(self): rename this test to not mention the 2nd push? @@ +604,5 @@ > + Action.objects.create( > + signoff=signoff, > + flag=Action.CANCELED, > + author=self.peter, > + ) from 597 to here can all go away.

Commit pushed to develop at https://github.com/mozilla/elmo https://github.com/mozilla/elmo/commit/abc2d94a54b61394c376231cda8683f8eb9bbfb6 fixes bug 668761 - all sign-offs in sign-off summary, r=Pike