Merge lp:~sinzui/launchpad/location-bug-262193 into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to only permit users to edit their own location.

    lp:~sinzui/launchpad/location-bug-262193
    Diff size: 460
    Launchpad bug: https://bugs.launchpad.net/bugs/262193
    Test command: ./bin/test -vvt "personlocation"
    Pre-implementation: my conscience
    Target release: 3.1.11

= Only permit users to edit their own location. =

After much debate about allowing users to set someone else's location, it
is clear from the usage statistics that the feature is rarely used.
Removing the feature will not have a significant impact on Launchpad.

== Rules ==

    * Restrict the permissions to edit a location to the owner or admin:
      launchpad.Edit
    * Remove the launchpad.Editlocation permission.
      \o/ one less exception permission in security.py
    * Remove any UI that suggests that a user can edit someone else's
      location. Most, maybe all the UI elements suggesting that a person
      can set another person's location was removed in 3.0.
    * Remove the notification email set when some edits another person's
      location.

== QA ==

    * On staging, verify that the page does not prompt you to set someone's
      location.
    * Verify that you cannot access the user's +editlocation page.

== Lint ==

Linting changed files:
  lib/canonical/launchpad/permissions.zcml
  lib/canonical/launchpad/security.py
  lib/lp/registry/configure.zcml
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/person.py
  lib/lp/registry/doc/personlocation.txt
  lib/lp/registry/model/person.py
  lib/lp/registry/stories/location/personlocation-edit.txt
  lib/lp/registry/stories/location/personlocation.txt

== Test ==

    * lib/lp/registry/doc/personlocation.txt
      * Revised the test to show that a user's location is only editable
        by himself or an admin.
      * Removed all tests that showed that a user could set someone else's
        location if the location was unset.
    * lib/lp/registry/stories/location/personlocation-edit.txt
      * Removed the tests that demonstrated a user could set another user's
        location if the location was unset.
    * lib/lp/registry/stories/location/personlocation.txt
      * Removed the test that implied a user could set another user's
        location.
      * Revised to the test to show that any user can see another user's
        location if it is public (the previous test implied the user had
        to be logged in).

== Implementation ==

    * lib/canonical/launchpad/emailtemplates/person-location-modified.txt
      * Deleted because it is not possible to set another user's location.

    * lib/canonical/launchpad/permissions.zcml
    * lib/canonical/launchpad/security.py
      * Removed launchpad.EditLocation.

    * lib/lp/registry/configure.zcml
    * lib/lp/registry/browser/configure.zcml
      * Replaced launchpad.EditLocation with launchpad.Edit.

    * lib/lp/registry/browser/person.py
      * Replaced launchpad.EditLocation with launchpad.Edit.
      * Since there is only one permission to access +editlocation, the
        field_names never change.

    * lib/lp/registry/model/person.py
      * Removed the notification about a user setting an...

[Eleanor Berger (intellectronica)]

simplicity rocks!

review: Approve (code)

=== removed file 'lib/canonical/launchpad/emailtemplates/person-location-modified.txt'
--- lib/canonical/launchpad/emailtemplates/person-location-modified.txt	2008-06-18 13:06:54 +0000
+++ lib/canonical/launchpad/emailtemplates/person-location-modified.txt	1970-01-01 00:00:00 +0000
@@ -1,19 +0,0 @@
-
-%(actor_browsername)s has updated your location and time zone information in
-Launchpad. This lets us present dates and times in your local time, and
-helps to coordinate team events that will span multiple time zones.
-
-Your information can be updated by any other user until you provide
-definitive data yourself, at which point the information is locked
-so that only you can edit it.
-
-Your details (now with map!): https://launchpad.net/~%(person)s
-%(actor_browsername)s's details: https://launchpad.net/~%(actor)s
-
-You can edit the location and time zone information for yourself at:
-
-    https://launchpad.net/~%(person)s/+editlocation
-
-Regards,
-
-The Launchpad team
 
=== modified file 'lib/canonical/launchpad/permissions.zcml'
--- lib/canonical/launchpad/permissions.zcml	2009-08-13 15:12:16 +0000
+++ lib/canonical/launchpad/permissions.zcml	2009-11-15 01:25:23 +0000
@@ -16,10 +16,6 @@
   <permission
     id="launchpad.Edit" title="Editing something" access_level="write" />
 
-  <permission
-    id="launchpad.EditLocation" title="Editing an object's location"
-    access_level="write" />
-
   <!-- Request large downloads, or other heavyweight jobs that are not
   semantically like "editing" and are not restricted to administrators.
   -->
 
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2009-11-13 05:13:07 +0000
+++ lib/canonical/launchpad/security.py	2009-11-15 01:25:23 +0000
@@ -624,38 +624,6 @@
         return person == user or user.inTeam(admins)
 
 
-class EditPersonLocation(AuthorizationBase):
-    permission = 'launchpad.EditLocation'
-    usedfor = IPerson
-
-    def checkAuthenticated(self, user):
-        """Anybody can edit a person's location until that person sets it.
-
-        Once a person sets his own location that information can only be
-        changed by the person himself or admins.
-        """
-        location = self.obj.location
-        if location is None:
-            # No PersonLocation entry exists for this person, so anybody can
-            # change this person's location.
-            return True
-
-        # There is a PersonLocation entry for this person, so we'll check its
-        # details to find out whether or not the user can edit them.
-        if (location.visible
-            and (location.latitude is None
-                 or location.last_modified_by != self.obj)):
-            # No location has been specified yet or it has been specified
-            # by a non-authoritative source (not the person himself), so
-            # anybody can change it.
-            return True
-        else:
-            admins = getUtility(ILaunchpadCelebrities).admin
-            # The person himself and LP admins can always change that person's
-            # location.
-            return user == self.obj or user.inTeam(admins)
-
-
 class ViewPersonLocation(AuthorizationBase):
     permission = 'launchpad.View'
     usedfor = IPersonLocation
 
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2009-11-11 10:30:23 +0000
+++ lib/lp/registry/browser/configure.zcml	2009-11-15 01:25:23 +0000
@@ -830,7 +830,7 @@
             name="+editlocation"
             for="lp.registry.interfaces.person.IPerson"
             class="lp.registry.browser.person.PersonEditLocationView"
-            permission="launchpad.EditLocation"
+            permission="launchpad.Edit"
             template="../templates/person-editlocation.pt"/>
         <browser:page
             name="+contactuser"
 
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2009-11-09 04:24:09 +0000
+++ lib/lp/registry/browser/person.py	2009-11-15 01:25:23 +0000
@@ -954,7 +954,7 @@
         text = 'Update Jabber IDs'
         return Link(target, text, icon='edit')
 
-    @enabled_with_permission('launchpad.EditLocation')
+    @enabled_with_permission('launchpad.Edit')
     def editlocation(self):
         target = '+editlocation'
         text = 'Set location and time zone'
@@ -5333,6 +5333,7 @@
     """Edit a person's location."""
 
     schema = PersonLocationForm
+    field_names = ['location', 'hide']
     custom_widget('location', LocationWidget)
 
     @property
@@ -5343,19 +5344,6 @@
     label = page_title
 
     @property
-    def field_names(self):
-        """See `LaunchpadFormView`.
-
-        If the user has launchpad.Edit on this context, then allow him to set
-        whether or not the location should be visible.  The field for setting
-        the person's location is always shown.
-        """
-        if check_permission('launchpad.Edit', self.context):
-            return ['location', 'hide']
-        else:
-            return ['location']
-
-    @property
     def initial_values(self):
         """See `LaunchpadFormView`.
 
 
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2009-11-12 12:01:22 +0000
+++ lib/lp/registry/configure.zcml	2009-11-15 01:25:23 +0000
@@ -805,7 +805,7 @@
                 permission="launchpad.View"
                 interface="lp.registry.interfaces.person.IPersonViewRestricted"/>
             <require
-                permission="launchpad.EditLocation"
+                permission="launchpad.Edit"
                 interface="lp.registry.interfaces.location.ISetLocation"/>
             <require
                 permission="launchpad.Edit"
 
=== modified file 'lib/lp/registry/doc/personlocation.txt'
--- lib/lp/registry/doc/personlocation.txt	2009-04-17 10:32:16 +0000
+++ lib/lp/registry/doc/personlocation.txt	2009-11-15 01:25:23 +0000
@@ -38,53 +38,22 @@
 requires that the user providing the information is passed as a
 parameter.
 
-We'll use jdub as a prolific source of location information for
-community members.
+A user cannot set another user's location.
 
     >>> jdub = personset.getByName('jdub')
     >>> login_person(jdub)
-
-First, jdub will provide information about cprov, who doesn't have any
-location data in the sampledata.
-
     >>> cprov = personset.getByName('cprov')
-    >>> print cprov.latitude
-    None
-    >>> print cprov.time_zone
-    None
     >>> cprov.setLocation(-43.0, -62.1, 'America/Sao_Paulo', jdub)
-    >>> print cprov.time_zone
-    America/Sao_Paulo
-
-When a person's location is changed by somebody else, we notify the
-person through email.
-
-    >>> from canonical.launchpad.database import PersonNotification
-    >>> notification = PersonNotification.selectOneBy(person=cprov)
-    >>> print notification.subject
-    Jeff Waugh updated your location and time zone
-
-    # Delete the notification so that it doesn't interfere in other tests.
-    >>> notification.destroySelf()
-
-And once cprov provides data for himself, jdub won't be able to edit it
-any more.
+    Traceback (most recent call last):
+    ...
+    Unauthorized:...
+
+A user can set his own location.
 
     >>> login_person(cprov)
     >>> cprov.setLocation(-43.2, -61.93, 'America/Sao_Paulo', cprov)
 
-    >>> login_person(jdub)
-    >>> cprov.setLocation(-43.0, -62.1, 'America/Sao_Paulo', jdub)
-    Traceback (most recent call last):
-    ...
-    Unauthorized:...
-
-When a user changes his own location, no notification is sent, though.
-
-    >>> PersonNotification.selectBy(person=cprov).count()
-    0
-
-But cprov can obviously still edit his own information. We need to deal
+cprov can change his location information. We need to deal
 with some floating point precision issues here, hence the rounding.
 
     >>> login_person(cprov)
@@ -92,7 +61,7 @@
     >>> abs(cprov.latitude + 43.52) < 0.001
     True
 
-And admins can, too.
+Admins can set a user's location.
 
     >>> admin = personset.getByName('name16')
     >>> login_person(admin)
@@ -100,20 +69,6 @@
     >>> abs(cprov.longitude + 62.1) < 0.001
     True
 
-If a user had provided only his time zone but not his location, other
-people would still be able to set his location.  That's the case of
-most our existing users, who set their time zone but hadn't set their
-locations (since there was no UI for that).
-
-    >>> salgado = personset.getByName('salgado')
-    >>> login_person(salgado)
-    >>> salgado.setLocation(None, None, 'America/Sao_Paulo', salgado)
-
-    >>> login_person(jdub)
-    >>> salgado.setLocation(-43.0, -62.1, 'America/Sao_Paulo', jdub)
-    >>> print salgado.latitude
-    -43.0
-
 We cannot store a location for a team, though.
 
     >>> guadamen = personset.getByName('guadamen')
@@ -214,11 +169,18 @@
 we provide a way for them to hide their location from other users. By
 default a person's location is visible.
 
+    >>> salgado = personset.getByName('salgado')
+    >>> login_person(salgado)
+    >>> salgado.setLocation(-43.0, -62.1, 'America/Sao_Paulo', salgado)
     >>> salgado.location.visible
     True
     >>> salgado.location.latitude
     -43...
 
+    >>> login_person(jdub)
+    >>> salgado.location.latitude
+    -43...
+
 But it can be changed through the setLocationVisibility() method. If the
 visibility is set to False, only the person himself will be able to see
 the location data except for time zone.
 
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2009-11-12 20:55:43 +0000
+++ lib/lp/registry/model/person.py	2009-11-15 01:25:23 +0000
@@ -551,20 +551,6 @@
                 person=self, time_zone=time_zone, latitude=latitude,
                 longitude=longitude, last_modified_by=user)
 
-        # Make a note that we need to tell this person that their
-        # information was updated by the user. We can only do this if we
-        # have a validated email address for this person.
-        if user != self and self.preferredemail is not None:
-            mail_text = get_email_template('person-location-modified.txt')
-            mail_text = mail_text % {
-                'actor': user.name,
-                'actor_browsername': user.displayname,
-                'person': self.name}
-            subject = '%s updated your location and time zone' % (
-                user.displayname)
-            getUtility(IPersonNotificationSet).addNotification(
-                self, subject, mail_text)
-
     # specification-related joins
     @property
     def assigned_specs(self):
 
=== modified file 'lib/lp/registry/stories/location/personlocation-edit.txt'
--- lib/lp/registry/stories/location/personlocation-edit.txt	2009-03-24 12:43:49 +0000
+++ lib/lp/registry/stories/location/personlocation-edit.txt	2009-11-15 01:25:23 +0000
@@ -1,9 +1,7 @@
 == Edit person location information ==
 
-The location information is editable by anybody (like a wiki), until the
-person provides location data for themselves. At that point it is only
-editable by people who have launchpad.Edit on the person, which is that
-person and admins.
+A person's location is only editable by people who have launchpad.Edit on
+the person, which is that person and admins.
 
     >>> login('[e-mail]')
     >>> zzz = factory.makePerson(
@@ -11,37 +9,15 @@
     ...     latitude=None, longitude=None, password='test')
     >>> logout()
 
-Anybody logged in can see the Location widget for a person who has no
-location set.
+A user cannot set another user's location.
 
     >>> nopriv_browser = setupBrowser(auth="Basic [e-mail]:test")
     >>> nopriv_browser.open('http://launchpad.dev/~zzz/+editlocation')
-    >>> find_tag_by_id(nopriv_browser.contents, "field.location.latitude")
-    <input...name="field.location.latitude" type="hidden"...
-
-Not only can we see the widgets, but anybody logged in can actually provide
-new location information.  Here we need to provide the location and time
-zone, but when accessed through a real browser the page will use Javascript
-to infer the time zone based on the location given.
-
-    >>> nopriv_browser.getControl(name='field.location.latitude').value = (
-    ...     '13.0')
-    >>> nopriv_browser.getControl(name='field.location.longitude').value = (
-    ...     '17.0')
-    >>> nopriv_browser.getControl(name='field.location.time_zone').value = [
-    ...     'Europe/Madrid']
-    >>> nopriv_browser.getControl('Update').click()
-    >>> login('[e-mail]')
-    >>> zzz.latitude
-    13.0
-    >>> zzz.longitude
-    17.0
-    >>> zzz.time_zone
-    u'Europe/Madrid'
-    >>> logout()
-
-If the person provides his own location information, then the data is
-locked, and random people can no longer edit it.
+    Traceback (most recent call last):
+    ...
+    Unauthorized:...
+
+A user can set his own location:
 
     >>> self_browser = setupBrowser(auth="Basic [e-mail]:test")
     >>> self_browser.open('http://launchpad.dev/~zzz/+editlocation')
@@ -53,12 +29,16 @@
     ...     'Europe/Madrid']
     >>> self_browser.getControl('Update').click()
 
-    >>> nopriv_browser.open('http://launchpad.dev/~zzz/+editlocation')
-    Traceback (most recent call last):
-    ...
-    Unauthorized:...
+    >>> login('[e-mail]')
+    >>> zzz.latitude
+    39.4...
+    >>> zzz.longitude
+    -0.4...
+    >>> zzz.time_zone
+    u'Europe/Madrid'
+    >>> logout()
 
-Of course, the person himself can continue to edit it:
+The user can change his location:
 
     >>> self_browser.open('http://launchpad.dev/~zzz/+editlocation')
     >>> self_browser.getControl(name='field.location.latitude').value
@@ -71,33 +51,21 @@
     '39.48'
 
 The +editlocation page also allows a person to change his location
-visibility, that is, whether or not others can see it. Unlike the actual
-location (which can be changed by anybody until the person himself sets
-it), the visibility can only be changed by the person himself.
+visibility, that is, whether or not others can see it. 
+
+    >>> nopriv_browser.open('http://launchpad.dev/~no-priv/+editlocation')
+    >>> nopriv_browser.getControl(
+    ...     'Hide my location details from others.').selected = True
+    >>> nopriv_browser.getControl('Update').click()
+    >>> nopriv_browser.url
+    'http://launchpad.dev/~no-priv'
+
+Once hidden, other users can't see it.
 
     >>> name12_browser = setupBrowser(auth="Basic [e-mail]:test")
-    >>> name12_browser.open('http://launchpad.dev/~no-priv/+editlocation')
-    >>> name12_browser.getControl('Hide my location details from others.')
-    Traceback (most recent call last):
-    ...
-    LookupError:...
-
-    >>> nopriv_browser.open('http://launchpad.dev/~no-priv/+editlocation')
-    >>> nopriv_browser.getControl(
-    ...     'Hide my location details from others.').selected = True
-    >>> nopriv_browser.getControl('Update').click()
-    >>> nopriv_browser.url
-    'http://launchpad.dev/~no-priv'
-
-Once hidden, other users can't see nor change it.
-
     >>> name12_browser.open('http://launchpad.dev/~no-priv')
     >>> print str(find_tag_by_id(name12_browser.contents, 'person_map_div'))
     None
-    >>> name12_browser.open('http://launchpad.dev/~no-priv/+editlocation')
-    Traceback (most recent call last):
-    ...
-    Unauthorized:...
 
 The person himself can still see and change it, though.
 
 
=== modified file 'lib/lp/registry/stories/location/personlocation.txt'
--- lib/lp/registry/stories/location/personlocation.txt	2009-09-16 17:56:17 +0000
+++ lib/lp/registry/stories/location/personlocation.txt	2009-11-15 01:25:23 +0000
@@ -8,30 +8,26 @@
     >>> zzz = factory.makePerson(name='zzz', time_zone='Africa/Maseru',
     ...                          latitude=None, longitude=None)
     >>> logout()
-    >>> nopriv_browser = setupBrowser(auth='Basic [e-mail]:test')
-    >>> nopriv_browser.open('http://launchpad.dev/~zzz')
-
-They should see a link to the +editlocation page (around the map image)
-and the person's time zone.
-
-    >>> editlink = nopriv_browser.getLink(url='+editlocation', index=1)
-    >>> editlink.text
-    '[IMG]'
-    >>> print extract_text(find_tag_by_id(nopriv_browser.contents, 'portlet-map'))
+
+Any person can see another person's time zone.
+
+    >>> anon_browser.open('http://launchpad.dev/~zzz')
+    >>> print extract_text(
+    ...     find_tag_by_id(anon_browser.contents, 'portlet-map'))
     Location
     Time zone: Africa/Maseru
 
-If a person has a location, they should have a little map portlet in their
-home page. We can't test all the google javascript, but we can make sure
+If a person has a location, there is a little map portlet in their
+profile page. We can't test all the google javascript, but we can make sure
 there's a map, and the scripts are loaded.
 
     >>> login('[e-mail]')
     >>> yyy = factory.makePerson(name='yyy', time_zone='Europe/London',
     ...                          latitude=52.2, longitude=0.3)
     >>> logout()
-    >>> nopriv_browser = setupBrowser(auth='Basic [e-mail]:test')
-    >>> nopriv_browser.open('http://launchpad.dev/~yyy')
-    >>> markup = str(nopriv_browser.contents)
+
+    >>> anon_browser.open('http://launchpad.dev/~yyy')
+    >>> markup = str(anon_browser.contents)
     >>> print extract_text(find_tag_by_id(markup, 'portlet-map'), skip_tags=[])
     Location
     Time zone: Europe/London...