1278539 - refactor history table backend code

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), defect, P3)

Description

[[e-mail] (:bhearsum)]

Balrog's history table backend code is a bit icky at the moment. Specifically:
* FieldView (https://github.com/mozilla/balrog/blob/cc7405f4d47f369b9f1c60380ae142432deb275c/auslib/admin/views/history.py) contains mappings of table names to table classes. This should probably be encapsulated elsewhere.
* DiffView doesn't even work for tables other than Releases, but is callable for them. This view is exclusively useful for releases.data, so we should probably make sure it's only possible to call it as part of that.
* The endpoint that lists the revisions of an object is reimplemented for each table, like https://github.com/mozilla/balrog/blob/4219832bf5c94b5faa39ec4c4ce8daca8fbf6a35/auslib/admin/views/rules.py#L171.

For these two points, I think making FieldView and DiffView subclassable may do the trick. We could specify the table-specific stuff in the base class, and explicitly specify routes for each table (would could give us the option of moving them into the table namespaces, like /rules, /releases, etc.). I think a new class in history.py that gets subclassed would probably work for the 3rd point as well - though I haven't exactly figured out how the table-specific parts could be factored out.

Comment 1

[Allan [:asilva]]

Hello Ben,I think that I can handle this. 
Do you have more considerations to make?

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Allan [:ex-dev] from comment #1)
> Hello Ben,I think that I can handle this. 
> Do you have more considerations to make?

Before you get started, it's a good idea to make sure that you can get all the existing tests passing with the instructions from https://github.com/mozilla/balrog. Once you can, I think the best approach to take is to rework the existing classes without changing any of the api endpoints or interfaces - that way the existing tests can validate your work.

Does comment #0 make sense to you, or do you need some further clarification or background? Feel free to join us in irc://irc.mozilla.org/#balrog if you have questions about this or other things.

Comment 3

[Allan [:asilva]]

Yeah, all tests passed!

One point: By subclassable, do you mean something like this from buildapi?

Base class:
https://github.com/mozilla-releng/build-buildapi/blob/master/buildapi/controllers/results.py

Specific classes:
https://github.com/mozilla-releng/build-buildapi/blob/master/buildapi/controllers/revision.py
https://github.com/mozilla-releng/build-buildapi/blob/master/buildapi/controllers/running.py

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Allan [:ex-dev] from comment #3)
> Yeah, all tests passed!
> 
> One point: By subclassable, do you mean something like this from buildapi?
> 
> Base class:
> https://github.com/mozilla-releng/build-buildapi/blob/master/buildapi/
> controllers/results.py
> 
> Specific classes:
> https://github.com/mozilla-releng/build-buildapi/blob/master/buildapi/
> controllers/revision.py
> https://github.com/mozilla-releng/build-buildapi/blob/master/buildapi/
> controllers/running.py

Similar to that, yes! You might be better using https://github.com/mozilla/balrog/blob/master/auslib/admin/views/scheduled_changes.py as a model though, since it is also in Balrog. You can see the base classes in that file, and some uses of them in https://github.com/mozilla/balrog/blob/master/auslib/admin/views/rules.py#L297 and https://github.com/mozilla/balrog/blob/master/auslib/admin/base.py#L98.

Comment 5

[Allan [:asilva]]

Hello Ben, 
Can you take a look in this commit bellow, this is my approach to make the FieldView subclassable and specific only on release contexts?  Tell me if changes is needed.

https://github.com/ex-Dev/balrog/commit/530a45137903d874d22352006ff5e6929da886a9

I did not open a PR yet, because I'm working on HistoryAdminView (3rd point) case. I did not found a viable approach yet.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Allan [:ex-dev] from comment #5)
> Hello Ben, 
> Can you take a look in this commit bellow, this is my approach to make the
> FieldView subclassable and specific only on release contexts?  Tell me if
> changes is needed.
> 
> https://github.com/ex-Dev/balrog/commit/
> 530a45137903d874d22352006ff5e6929da886a9
> 
> I did not open a PR yet, because I'm working on HistoryAdminView (3rd point)
> case. I did not found a viable approach yet.

(In the future, just open a PR even if you're not done yet - it's much easier to keep track of review comments there.)

It looks like you're on the right track. I like that DiffView is now release-specific (because it is in reality, anyways), and passing the table objects explicitly in subclasses should reduce runtime errors.

I see you're removing some tests - please make sure you add new ones in their place! You'll probably need to put them in test_releases.py, test_permissions.py, etc. now.

Comment 7

[Allan [:asilva]]

Attachment: PR for review

Comment 8

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/1ca1d471a96f11ef0beb3efc6df00a097205ecad
Bug 1278539 - refactory history api backend code (#230). r=bhearsum

Comment 9

[[e-mail] (:bhearsum)]

This is in production and appears to be working well. Thanks Allan!