Merge lp:~rharding/launchpad/related_projects_1063272 into lp:launchpad
Status: | Merged |
---|---|
Approved by: | Richard Harding |
Approved revision: | no longer in the source branch. |
Merged at revision: | 16185 |
Proposed branch: | lp:~rharding/launchpad/related_projects_1063272 |
Merge into: | lp:launchpad |
Diff against target: |
520 lines (+269/-33) 8 files modified
lib/lp/registry/browser/person.py (+10/-5) lib/lp/registry/doc/person-account.txt (+3/-3) lib/lp/registry/interfaces/person.py (+18/-2) lib/lp/registry/interfaces/product.py (+7/-0) lib/lp/registry/model/person.py (+109/-18) lib/lp/registry/model/product.py (+9/-0) lib/lp/registry/tests/test_person.py (+97/-5) lib/lp/registry/tests/test_product.py (+16/-0) |
To merge this branch: | bzr merge lp:~rharding/launchpad/related_projects_1063272 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Brad Crittenden (community) | code | Approve | |
Review via email:
|
Commit message
Filter affiliatedPillars based on access grants to hide non-visible products.
Description of the change
= Summary =
The person getAffiliatedPi
not visible to the user. This adds the same checks as
product.
== Pre Implementation ==
Talked with Orange about if this could/should be converted to storm or
performed with raw sql adjustments. The storm/union/sorting issues kept it as
raw sql at this time.
Talked with Curtis and Deryck around handling the account deactivation issue
this ran smack into.
== Implementation Notes ==
Due to the nature of the query here, with unions that must be in a specific
sorted order, the product method could not be used. It's logic was turned into
raw sql and used to filter out the product part of the union.
This makes sure that commercial admins and admins maintain access/visibility
by passing in the current viewing user to the underlying methods in order to
check for permissions.
_genAffiliatedP
part of the union is to be done. It uses the old query if you're an
admin/commercial admin, however it filters based on granted products
otherwise. See product.
construct this query.
As a side effect, we ran into the fact that account deactivation uses
getAffiliatedPi
The decision was made that accounts that are owners of non-public products
cannot be deactivated. This needed to be added to view validation. In order to
prevent duplicate code, the validation was added to the person model. It
checks for a list of non-public products owned by the user. For validation
purposes, we needed two methods. One that's just a boolean T/F, the second
however, is needed to get readable error message back to the view to be placed
in front of the user. Because of this we've added both canDeactivateUser and
canDeactivateUs
The validation checking in Person required a new query on Product so that we
can easily and quickly get the list of non-public products. We needed to do a
full query to get the list so we can provide the names of those products to
the user in the validation error message.
The final trick was to add a new can_deactivate kwarg so that we can prevent
the query being hit twice, once in the view and once in the model. Since the
view did the check it can let the model know it's been sanity checked already.
There's a drive by change requested by Curtis. There's no sense setting the bug supervisor or the driver to registry experts. They are nullable values. So we set them to null and only change the owner to help with registry spam.
== QA ==
Log in as a user and create a private product. Then visit the user's
+related-projects page and it should load just fine.
Log out and as an anonymous user also load the same url without issue. No
private products should be visible to the anonymous user.
Deactivating the user with the private product should not be permitted. It
must be re-assigned to another user before deactivation is permitted.
== Tests ==
registry/
registry/
Hi Rick,
Some comments on your branch:
* In genAffiliatedPr oductSql there is a lot of repetition in the queries that could be factored out so you DRY as the first query you return is a the same as the final one minus one clause. Give a shot at refactoring if you don't mind.
* Thanks for the nice error messages when the account cannot be deactivated.
* The tests are clear and easy to follow. Thanks.