Closed Bug 761662 Opened 12 years ago Closed 12 years ago

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

Categories

(Webtools Graveyard :: Elmo, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Pike, Assigned: peterbe)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 761669
Priority: -- → P2
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.
Assignee: nobody → peterbe
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.
Attachment #651577 - Flags: review?(l10n)
Blocks: 658656
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.
Attachment #651577 - Flags: review?(l10n) → review-
Was this what you had in mind?
Attachment #651577 - Attachment is obsolete: true
Attachment #682076 - Flags: review?(l10n)
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?
Attachment #682076 - Flags: review?(l10n) → feedback+
Blocks: 668761
Attached patch tests somethingSplinter Review
After all, this is an refactoring patch. The test makes sure it works and that `de` changesets come out in the right order.
Attachment #682076 - Attachment is obsolete: true
Attachment #699518 - Flags: review?(l10n)
Comment on attachment 699518 [details] [diff] [review] tests something Review of attachment 699518 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looking good, r=me.
Attachment #699518 - Flags: review?(l10n) → review+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.1
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: