launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Needs Information code

After reading the code, I think the branch creation rule is:
    When The branch sharing policy is EMBARGOED_OR_PROPRIETARY,
    only users granted PROPRIETARY may create branches
    and the branch information type is EMBARGOED.

Thus both ~oem-solutions-group and ~project-team can both create branch, and those branches will be EMBARGOED. Members of ~oem-solutions-group and ~pmtteam will see all branches because all EMBARGOED is shared with them, but ~project-team can only see the branches they are subscribed to, which will be the branches they push.

^ This is sound, but is my interpretation correct?

I do not think we need to import COMMERCIAL_ADMIN_EMAIL on line 38. I don't intend to setup OEM's projects..they will. So I think
        product.setBranchSharingPolicy(
            BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY, comadmin)
should be
        product.setBranchSharingPolicy(
            BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY, product.owner)
to ensure the people who will do this can do so.

Per William's remarks we want to filter out Embargoed on lines 85 and 85 to ensure it does not show up in the UI.

I am also uncomfortable with the use of with admin_logged_in() on line 140 because I don't think the project shares with ~admins (and I am certain they wont be doing what is demonstrated in the test). We could explicitly check this case using a user that is only shared PROPRIETARY information, but the product owner is also a valid actor.
    with person_logged_in(namespace.product.owner):
-- 
https://code.launchpad.net/~wallyworld/launchpad/embargoed-information-type-1036187/+merge/119837
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.