Merge lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated into lp:launchpad
Status: | Merged |
---|---|
Merged at revision: | 12943 |
Proposed branch: | lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated |
Merge into: | lp:launchpad |
Diff against target: |
237 lines (+141/-30) 3 files modified
lib/lp/app/security.py (+17/-8) lib/lp/app/tests/test_security.py (+114/-22) lib/lp/testing/__init__.py (+10/-0) |
To merge this branch: | bzr merge lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+59364@code.launchpad.net |
Commit message
Prevent foot shooting with forwardCheckAut
Description of the change
= Summary =
To fix the bug is really just a two-line fix. Quite straight forward.
The new method forwardCheckAut
defined behavior. This branch also adds that but it also adds some
complexity.
== Proposed fix ==
In lp.app.security:
- Introduce checkPermission
with FakeMethod during testing.
- Call checkPermisionI
- Replace getAdapter with queryAdapter and return False when the
adapter is not found.
- Add an assertion to avoid loops.
In lp.add.
- Update the whole test to use FakeMethod and ZopelessDataseL
- Add tests for forwardCheckAut
because it involves registering an adapter.
In lp.testing
- Introduce assertVectorEqual to test multiple values at the same time.
This formalizes a pattern that I like to use by making sure all values
are asserted. It could probably be converted to use matchers and even
be added to testtools but this implementation was the quickest for me.
(Go and read what zip() does ... ;-)
== Pre-implementation notes ==
I had talked about the fix with William after he had actually been
bitten by the bug.
== Implementation details ==
I thought I could use directlyProvide
seems not, so I had to introduce the dummy implementation of the
interface.
== Tests ==
bin/test -vvcm lp.app.
== Demo and Q/A ==
No QA possible.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
lib/lp/
lib/lp/
This looks good, but I don't like assertVectorEqual() very much :-/
One minor gripe is that it's not much of an improvement over doing two
assertEqual() calls, but the main problem is that the error messages
won't make sense because they'll contain expected_vector and
observed_vector. In short I think it's better to simply do something
like:
(1, 1),
because then the error messages will make sense.