launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/location-api-0 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #657109 SCA-user is rejected by LP API as unauthorized
  https://bugs.launchpad.net/bugs/657109


This is my branch to not snapshot the person location.

    lp:~sinzui/launchpad/location-api-0
    Diff size: 58
    Launchpad bug: https://bugs.launchpad.net/bugs/657109
    Test command: ./bin/test -vv  -t test_person_snapshot
    Pre-implementation: no one
    Target release: 10.11


Do not snapshot the person location
------------------------------------

The SCA logs show a user's access to a PPA failed  because the User's
location was in the snapshot of the user change. Only the user can see
that information, and that information was not changed by the app or
the user. Snapshot is collecting too much unnecessary information.

Snapshotting is used for event notification. Since only the user can see
and change his or her location, there are no notifications about this change,
and there is no need to ever snapshot it.

Rules
-----

    * Use doNotSnapshot() in the location interface to ensure it is not
      used.


QA
--

This may not be possible to test. The scenario requires running a script
that acts for a user with a hidden location to subscribe the user to a p3a,
then calling subscriber.getArchiveSubscriptionURL(archive=p3a)


Lint
----

Linting changed files:
  lib/lp/registry/interfaces/location.py
  lib/lp/registry/tests/test_person.py


Test
----

Updated lib/lp/registry/tests/test_person.py to verify that the three location
fields are not snapshotted.


Implementation
--------------

Updated lib/lp/registry/interfaces/location.py to exclude the three location
fields from snapshots.
-- 
__Curtis C. Hovey_________
http://launchpad.net/
-- 
https://code.launchpad.net/~sinzui/launchpad/location-api-0/+merge/39520
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/location-api-0 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/interfaces/location.py'
--- lib/lp/registry/interfaces/location.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/interfaces/location.py	2010-10-28 12:41:52 +0000
@@ -19,6 +19,7 @@
     'ISetLocation',
     ]
 
+from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.declarations import (
     call_with,
     export_write_operation,
@@ -45,16 +46,16 @@
 class IHasLocation(Interface):
     """An interface supported by objects with a defined location."""
 
-    latitude = exported(
+    latitude = exported(doNotSnapshot(
         Float(title=_("The latitude of this object."),
-              required=False, readonly=True))
-    longitude = exported(
+              required=False, readonly=True)))
+    longitude = exported(doNotSnapshot(
         Float(title=_("The longitude of this object."),
-              required=False, readonly=True))
-    time_zone = exported(
+              required=False, readonly=True)))
+    time_zone = exported(doNotSnapshot(
         Choice(title=_('The time zone of this object.'),
                required=False, readonly=True,
-               vocabulary='TimezoneName'))
+               vocabulary='TimezoneName')))
 
 
 class IObjectWithLocation(Interface):
@@ -65,7 +66,7 @@
 
 class ILocationRecord(IHasLocation):
     """A location record, which may be attached to a particular object.
-    
+
     The location record contains additional information such as the date the
     location data was recorded, and by whom.
     """

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-10-26 15:47:24 +0000
+++ lib/lp/registry/tests/test_person.py	2010-10-28 12:41:52 +0000
@@ -373,7 +373,8 @@
             'all_members_prepopulated', 'approvedmembers',
             'deactivatedmembers', 'expiredmembers', 'inactivemembers',
             'invited_members', 'member_memberships', 'pendingmembers',
-            'proposedmembers', 'unmapped_participants',
+            'proposedmembers', 'unmapped_participants', 'longitude',
+            'latitude', 'time_zone',
             )
         snap = Snapshot(self.myteam, providing=providedBy(self.myteam))
         for name in omitted: