Dehorusify ProfileController #2274

nickstenning · Jun 3, 2015

As a first step towards remove horus completely from our accounts system, here's the ProfileController reworked to not depend on horus. Along the way I've fixed the following bugs:

logout not working on master (also fixed temporarily by #2270)
update anyone's subscriptions simply by picking a subscription id

The tests are now an order of magnitude easier to write.

Repository health decreased by 0.78% when pulling f6f04a1 on dehorusify-profilecontroller into 06278fb on master.

64 new problems were found (including 0 errors and 29 code smells).
35 problems were fixed (including 0 errors and 14 code smells).

landscape-bot · Jun 3, 2015

Repository health decreased by 0.78% when pulling f6f04a1 on dehorusify-profilecontroller into 06278fb on master.

64 new problems were found (including 0 errors and 29 code smells).
35 problems were fixed (including 0 errors and 14 code smells).

seanh · Jun 3, 2015

seanh reviewed Jun 3, 2015

View changes

h/accounts/test/views_test.py

seanh · Jun 3, 2015

seanh reviewed Jun 3, 2015

View changes

seanh · Jun 3, 2015

h/accounts/views.py

@@ -64,6 +60,16 @@ def ajax_form(request, result):
    return result


+def validate_form(form, data):
+    """Validate POST payload data for a form."""
+    try:


seanh · Jun 3, 2015

seanh reviewed Jun 3, 2015

View changes

seanh · Jun 3, 2015

h/accounts/views.py

 @view_auth_defaults
 @view_config(attr='edit_profile', route_name='edit_profile')
 @view_config(attr='disable_user', route_name='disable_user')
 @view_config(attr='profile', route_name='profile')
-class ProfileController(horus.views.ProfileController):
+class ProfileController(object):


Yes :) I love classes that inherit from just object

seanh · Jun 3, 2015

For what it's worth the value in wrapping the tests for a given function in a class is making it easier to find the tests for that function and know that you've found all of them - they're all in this class. Otherwise it can be difficult, when dealing with large test modules. Agree on not using setup and teardown or self though.

For what it's worth the value in wrapping the tests for a given function in a class is making it easier to find the tests for that function and know that you've found all of them

I get that, but don't you achieve the same thing by:

putting them all in the same place
giving them good names? (i.e. test_edit_profile_does_foo, test_edit_profile_does_bar)

nickstenning · Jun 3, 2015

For what it's worth the value in wrapping the tests for a given function in a class is making it easier to find the tests for that function and know that you've found all of them

I get that, but don't you achieve the same thing by:

putting them all in the same place
giving them good names? (i.e. test_edit_profile_does_foo, test_edit_profile_does_bar)

edit_profile() couild do with being broken up into multiple methods, but it was already as long before this pr

seanh · Jun 3, 2015

edit_profile() couild do with being broken up into multiple methods, but it was already as long before this pr

This all looks good to me, code is much simpler and easier to understand.

I'm a little disturbed by how long edit_profile() is and how few tests we have for edit_profile() and ProfileSchema, that isn't this PR's fault, but it might be good to prioritise writing more tests for those.

seanh · Jun 3, 2015

This all looks good to me, code is much simpler and easier to understand.

I'm a little disturbed by how long edit_profile() is and how few tests we have for edit_profile() and ProfileSchema, that isn't this PR's fault, but it might be good to prioritise writing more tests for those.

For what it's worth the value in wrapping the tests for a given function in a class is making it easier to find the tests for that function and know that you've found all of them
I get that, but don't you achieve the same thing by:

putting them all in the same place
giving them good names? (i.e. test_edit_profile_does_foo, test_edit_profile_does_bar)

Sort of. But it's the difference between scrolling through a long tests file and noticing "Hey, the function names are starting with test_edit_profile_ now" versus seeing a class with a docstring that says "This is where all the tests for edit_profile() go", which I think is clearer. The indentation difference and ability to search for "class " makes it easier to jump around in and get an overview of the file.

I'm happy enough either way though, in past projects I've tried to insist on not having test classes so that people wouldn't be tempted to start using self.

seanh · Jun 3, 2015

For what it's worth the value in wrapping the tests for a given function in a class is making it easier to find the tests for that function and know that you've found all of them
I get that, but don't you achieve the same thing by:

putting them all in the same place
giving them good names? (i.e. test_edit_profile_does_foo, test_edit_profile_does_bar)

Sort of. But it's the difference between scrolling through a long tests file and noticing "Hey, the function names are starting with test_edit_profile_ now" versus seeing a class with a docstring that says "This is where all the tests for edit_profile() go", which I think is clearer. The indentation difference and ability to search for "class " makes it easier to jump around in and get an overview of the file.

I'm happy enough either way though, in past projects I've tried to insist on not having test classes so that people wouldn't be tempted to start using self.

I think some of landscape's complaints about the tests are actually valid: unused variables, unused imports, == True, line too long. But not all the missing docstring and other docstring complaints, not the redefined name or the unused arguments. And I'm not sure if the valid complaints are really introduced by this pr.

seanh · Jun 3, 2015

I think some of landscape's complaints about the tests are actually valid: unused variables, unused imports, == True, line too long. But not all the missing docstring and other docstring complaints, not the redefined name or the unused arguments. And I'm not sure if the valid complaints are really introduced by this pr.

@tilgovi

I'm happy with this, recommend @tilgovi has a quick look as I think he understands horus, colander and deform better than I do.

seanh · Jun 3, 2015

I'm happy with this, recommend @tilgovi has a quick look as I think he understands horus, colander and deform better than I do.

@seanh

I'd say if test files are getting so long that it becomes a pain to identify and find a subset of them that you need it might mean the tested module has grown similarly too large and needs to be broken up. I'm not familiar with the pain @seanh is suggestion, but let's watch for it to occur.

tilgovi · Jun 3, 2015

I'd say if test files are getting so long that it becomes a pain to identify and find a subset of them that you need it might mean the tested module has grown similarly too large and needs to be broken up. I'm not familiar with the pain @seanh is suggestion, but let's watch for it to occur.

@nickstenning

I'm general a fan of fewer classes in most things, so I'm tempted to agree with @nickstenning here for now.

tilgovi · Jun 3, 2015

I'm general a fan of fewer classes in most things, so I'm tempted to agree with @nickstenning here for now.

tilgovi · Jun 3, 2015

tilgovi reviewed Jun 3, 2015

View changes

tilgovi · Jun 3, 2015

h/accounts/views.py

-        except _InvalidEditProfileRequestError as err:
-            return dict(errors=err.errors)
+        """Handle POST payload from profile update form."""
+        if self.request.method != 'POST':


This can also be done by adding the request_method predicate to the view decorator, but this is okay.

tilgovi · Jun 3, 2015

tilgovi reviewed Jun 3, 2015

View changes

tilgovi · Jun 3, 2015

h/accounts/views.py

-            return dict(errors=e.error.children)
+        password = appstruct.get('password')
+        if password:
+            user.password = password


I think we're forgetting to add the updated user to the db session so it commits at the end of the request.

You don't need to add objects that came from the database in the first place. sqlalchemy does dirty checking. You only need to add new objects.

And here's a reference for that assertion :)

tilgovi · Jun 3, 2015

tilgovi reviewed Jun 3, 2015

View changes

tilgovi · Jun 3, 2015

h/accounts/views.py

        if user:
            # TODO: maybe have an explicit disabled flag in the status
-            user.password = self.User.generate_random_password()
-            self.db.add(user)


I think this needs to be here.

tilgovi · Jun 3, 2015

tilgovi reviewed Jun 3, 2015

View changes

tilgovi · Jun 3, 2015

h/accounts/views.py

@@ -367,7 +345,6 @@ def unsubscribe(self):
        subscription = Subscriptions.get_by_id(request, subscription_id)
        if subscription:
            subscription.active = False
-            self.db.add(subscription)


Totally owe you 🍻 for this one. Makes me so happy.

Only thing missing is actually adding the changes to the db :).

tilgovi · Jun 3, 2015

Totally owe you 🍻 for this one. Makes me so happy.

Only thing missing is actually adding the changes to the db :).

I'll just take a quick look through some of the things Sean raised, so don't merge it juuust yet.

nickstenning · Jun 3, 2015

I'll just take a quick look through some of the things Sean raised, so don't merge it juuust yet.

Okay. I've also just checked this locally and was able to change my password, so I'm confused about SQLAlchemy sessions and the need for an explicit add. If you can clarify why it's not needed that would help me.

tilgovi · Jun 3, 2015

Okay. I've also just checked this locally and was able to change my password, so I'm confused about SQLAlchemy sessions and the need for an explicit add. If you can clarify why it's not needed that would help me.

http://docs.sqlalchemy.org/en/latest/orm/session_state_management.html#quickie-intro-to-object-states

Right. I was mistaken because I'd seen excessive use of add in other code, I think.

If the object came from the DB it's already persistent and its attributes are managed so when the transaction commits at the end of the request it's done.

tilgovi · Jun 3, 2015

http://docs.sqlalchemy.org/en/latest/orm/session_state_management.html#quickie-intro-to-object-states

Right. I was mistaken because I'd seen excessive use of add in other code, I think.

If the object came from the DB it's already persistent and its attributes are managed so when the transaction commits at the end of the request it's done.

Oh, sorry, I did already link to some sqla docs in the line comment above.

nickstenning · Jun 3, 2015

Oh, sorry, I did already link to some sqla docs in the line comment above.

No worries.

tilgovi · Jun 3, 2015

No worries.

@seanh

LGTM. Whenever @seanh is happy.

tilgovi · Jun 3, 2015

LGTM. Whenever @seanh is happy.

LGTM, I'll leave it up to Nick to correct as many of my points as he wants, none of them were blockers

seanh · Jun 3, 2015

LGTM, I'll leave it up to Nick to correct as many of my points as he wants, none of them were blockers

Ready for merge when the lights go green.

nickstenning · Jun 3, 2015

Ready for merge when the lights go green.

Repository health decreased by 0.32% when pulling 4beb481 on dehorusify-profilecontroller into 06278fb on master.

47 new problems were found (including 0 errors and 21 code smells).
36 problems were fixed (including 0 errors and 15 code smells).

landscape-bot · Jun 3, 2015

Repository health decreased by 0.32% when pulling 4beb481 on dehorusify-profilecontroller into 06278fb on master.

47 new problems were found (including 0 errors and 21 code smells).
36 problems were fixed (including 0 errors and 15 code smells).

tilgovi · Jun 3, 2015

Super good.

nickstenning referenced this pull request Jun 3, 2015
Merged
Don't run prospector over test packages #2276

Minor cleanups in h.accounts.test.views_test
Remove some unused imports, fix some long lines and unused variables, etc.

Loading status checks…

4beb481

tilgovi added a commit that referenced this pull request Jun 3, 2015

Merge pull request #2274 from hypothesis/dehorusify-profilecontroller
Dehorusify ProfileController

c47131f

tilgovi merged commit c47131f into master Jun 3, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

tilgovi deleted the dehorusify-profilecontroller branch Jun 3, 2015

hypothesis/h

Join GitHub today

Dehorusify ProfileController #2274

Conversation

nickstenning commented Jun 3, 2015

nickstenning referenced this pull request Jun 3, 2015

nickstenning added some commits Jun 2, 2015

This comment has been minimized.

landscape-bot commented Jun 3, 2015

seanh reviewed Jun 3, 2015 View changes

seanh reviewed Jun 3, 2015 View changes

This comment has been minimized.

seanh reviewed Jun 3, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

seanh commented on f5cc263 Jun 3, 2015

This comment has been minimized.

nickstenning commented Jun 3, 2015

This comment has been minimized.

seanh commented Jun 3, 2015

This comment has been minimized.

seanh commented Jun 3, 2015

This comment has been minimized.

seanh commented Jun 3, 2015

This comment has been minimized.

seanh commented Jun 3, 2015

This comment has been minimized.

seanh commented Jun 3, 2015

This comment has been minimized.

tilgovi commented Jun 3, 2015

This comment has been minimized.

tilgovi commented Jun 3, 2015

tilgovi reviewed Jun 3, 2015 View changes

This comment has been minimized.

tilgovi reviewed Jun 3, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tilgovi reviewed Jun 3, 2015 View changes

This comment has been minimized.

tilgovi reviewed Jun 3, 2015 View changes

This comment has been minimized.

This comment has been minimized.

tilgovi commented Jun 3, 2015

This comment has been minimized.

nickstenning commented Jun 3, 2015

This comment has been minimized.

tilgovi commented Jun 3, 2015

This comment has been minimized.

tilgovi commented Jun 3, 2015

This comment has been minimized.

nickstenning commented Jun 3, 2015

This comment has been minimized.

tilgovi commented Jun 3, 2015

This comment has been minimized.

tilgovi commented Jun 3, 2015

This comment has been minimized.

seanh commented Jun 3, 2015

This comment has been minimized.

nickstenning commented Jun 3, 2015

This comment has been minimized.

landscape-bot commented Jun 3, 2015

This comment has been minimized.

tilgovi commented on 4beb481 Jun 3, 2015

tilgovi added a commit that referenced this pull request Jun 3, 2015

Hide details View details tilgovi merged commit c47131f into master Jun 3, 2015 2 checks passed

2 checks passed

tilgovi deleted the dehorusify-profilecontroller branch Jun 3, 2015

seanh reviewed Jun 3, 2015

View changes

seanh reviewed Jun 3, 2015

View changes

seanh reviewed Jun 3, 2015

View changes

seanh commented on `f5cc263` Jun 3, 2015

tilgovi reviewed Jun 3, 2015

View changes

tilgovi reviewed Jun 3, 2015

View changes

tilgovi reviewed Jun 3, 2015

View changes

tilgovi reviewed Jun 3, 2015

View changes

tilgovi commented on `4beb481` Jun 3, 2015

tilgovi merged commit `c47131f` into master Jun 3, 2015
2 checks passed