761662 - [shipping] move rowcollector logic into class-based view, and test it

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Axel Hecht [:Pike]]

The rowcollector is only used for the signoff view now that data1.1. is fixed.

We should move that into the class-based view, and chunk things up so that they can be tested as methods. shipping.views.release.MigrateAppversions might be a good example for that.

Comment 1

[Peter Bengtsson [:peterbe]]

Note-to-self:

1. Move signoff() into a class based view.

2. Move annotated_pushes() into said class

3. Copy the functionality that _RowCollector does (rename __init__() to something like collect())

4. Amend all appropriate unit tests.

5. (if time allows), consider refactoring annotated_pushes() itself so it too can be split up into sub-methods that can have their own unit tests.

Comment 2

[Peter Bengtsson [:peterbe]]

Attachment: refactored signoff view and moved annotated_pushes and _RowCollector into it

Notice the XXX comment in the bottom of test_signoff.py :)

I stared at it but made little substantial progress to dissect the hot mess that annotated_pushes(), collect() and wrapup() is. Feel free to r- based on that but then please guide and help out with what can be refactored out and unit tested separately.

Comment 3

[Axel Hecht [:Pike]]

Comment on attachment 651577 [details] [diff] [review]
refactored signoff view and moved annotated_pushes and _RowCollector into it

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

Sorry for the lag, I know we talked about this a while back, but I didn't get to write down my thoughts.

Generally, I'd like to make dependencies of methods as explicit as possible, thus they should be close to static. That makes writing tests so much easier, among other things.

In this particular code, I broke that concept in the collect/wrapup combo, and use self as a closure. It'd be great if you could add comments to that extent.

Other than that, I think the code move is OK, we may be able to do different factorizations in follow ups, too. I hinted at one where I see an option to factor handling out into a method, and explicitly test it. I suspect there'd be more in the collect code path.

Giving this an r-, as we'll want an updated patch for this.

::: apps/shipping/views/signoff.py
@@ +34,5 @@
> +        self.lang = get_object_or_404(Locale, code=locale_code)
> +        context = self.get_context_data()
> +        return self.render_to_response(context)
> +
> +    def get_context_data(self):

Can we add the dependencies here? aka, appver and lang as arguments instead of getting them from self.

@@ +55,5 @@
> +        if real_av != self.appver.code and accepted is not None:
> +            # we're falling back, add the accepted push to the table
> +            fallback = accepted
> +        else:
> +            fallback = None

This sounds like an interesting point to cut, factor and test.

Comment 4

[Peter Bengtsson [:peterbe]]

Attachment: appver and lang arguments to annotated_pushes() instead

Was this what you had in mind?

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 682076 [details] [diff] [review]
appver and lang arguments to annotated_pushes() instead

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

Yes, like this.

I'm moving this to a feedback+, mostly because I'd not want "I don't know what I'm testing" in the tree ;-)

Also, I guess this patch is gonna have a hard time to apply, given that we removed its context with the etags in the tests in the meantime.

::: apps/shipping/tests/test_signoff.py
@@ +333,5 @@
> +            fallback,
> +            locale,
> +            self.av
> +        )
> +        # XXX: I'm not entirely sure what I'm testing here.

I don't know ad-hoc, but the different locales should probably have different values here. Maybe test_status_json can guide the way?

Comment 6

[Peter Bengtsson [:peterbe]]

Attachment: tests something

After all, this is an refactoring patch. The test makes sure it works and that `de` changesets come out in the right order.

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 699518 [details] [diff] [review]
tests something

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

Thanks, looking good, r=me.

Comment 8

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/09c9f11258cb59d47e50366668de6bfcb47d5419
bug 761662 - refactored signoff view and moved annotated_pushes and _RowCollector into it, r=Pike