← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~cjwatson/launchpad/sprint-physical into lp:launchpad

 

Colin Watson has proposed merging lp:~cjwatson/launchpad/sprint-physical into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1391281 in Launchpad itself: "Support UDS (UOS) virtual format for sprint attendance"
  https://bugs.launchpad.net/launchpad/+bug/1391281

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/sprint-physical/+merge/242658

== Summary ==

There's no way to have a sprint that doesn't ask attendees whether they're attending physically or remotely, even if it's an entirely virtual sprint with no physical location.  The latter is now the common case for Ubuntu sprints.

== Proposed fix ==

Add Sprint.is_physical, and only show UI for SprintAttendance.is_physical if Sprint.is_physical is true.

== LOC Rationale ==

+61, which isn't too bad.  I suspect that switching from doctests to unittests would address a lot of this, but I honestly couldn't face it ...

== Tests ==

bin/test -vvct sprint

== Demo and Q/A ==

Make sure that existing sprints remain physical and behave as before; make sure that sprints can be created as non-physical or edited as such, and that in that case no physical/remote option is given for attendance.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/sprint-physical into lp:launchpad.
=== modified file 'lib/lp/blueprints/browser/sprint.py'
--- lib/lp/blueprints/browser/sprint.py	2014-11-24 01:20:26 +0000
+++ lib/lp/blueprints/browser/sprint.py	2014-11-24 13:07:07 +0000
@@ -253,7 +253,8 @@
     schema = ISprint
     label = "Register a meeting"
     field_names = ['name', 'title', 'summary', 'home_page', 'driver',
-                   'time_zone', 'time_starts', 'time_ends', 'address',
+                   'time_zone', 'time_starts', 'time_ends', 'is_physical',
+                   'address',
                    ]
     custom_widget('summary', TextAreaWidget, height=5)
     custom_widget('time_starts', DateTimeWidget, display_zone=False)
@@ -292,6 +293,7 @@
             time_zone=data['time_zone'],
             time_starts=data['time_starts'],
             time_ends=data['time_ends'],
+            is_physical=data['is_physical'],
             address=data['address'],
             )
         self.request.response.addInfoNotification('Sprint created.')
@@ -321,7 +323,8 @@
     label = "Edit sprint details"
 
     field_names = ['name', 'title', 'summary', 'home_page', 'driver',
-                   'time_zone', 'time_starts', 'time_ends', 'address',
+                   'time_zone', 'time_starts', 'time_ends', 'is_physical',
+                   'address',
                    ]
     custom_widget('summary', TextAreaWidget, height=5)
     custom_widget('time_starts', DateTimeWidget, display_zone=False)

=== modified file 'lib/lp/blueprints/browser/sprintattendance.py'
--- lib/lp/blueprints/browser/sprintattendance.py	2012-01-01 02:58:52 +0000
+++ lib/lp/blueprints/browser/sprintattendance.py	2014-11-24 13:07:07 +0000
@@ -28,13 +28,20 @@
 class BaseSprintAttendanceAddView(LaunchpadFormView):
 
     schema = ISprintAttendance
-    field_names = ['time_starts', 'time_ends', 'is_physical']
     custom_widget('time_starts', DateTimeWidget)
     custom_widget('time_ends', DateTimeWidget)
     custom_widget(
         'is_physical', LaunchpadBooleanRadioWidget, orientation='vertical',
         true_label="Physically", false_label="Remotely", hint=None)
 
+    @property
+    def field_names(self):
+        """Return the list of field names to display."""
+        field_names = ['time_starts', 'time_ends']
+        if self.context.is_physical:
+            field_names.append('is_physical')
+        return field_names
+
     def setUpWidgets(self):
         LaunchpadFormView.setUpWidgets(self)
         tz = pytz.timezone(self.context.time_zone)
@@ -153,7 +160,7 @@
     @action(_('Register'), name='register')
     def register_action(self, action, data):
         time_starts, time_ends = self.getDates(data)
-        is_physical = data['is_physical']
+        is_physical = self.context.is_physical and data['is_physical']
         self.context.attend(self.user, time_starts, time_ends, is_physical)
 
 
@@ -161,7 +168,12 @@
     """A view used to register someone else's attendance at a sprint."""
 
     label = 'Register someone else'
-    field_names = ['attendee'] + list(BaseSprintAttendanceAddView.field_names)
+
+    @property
+    def field_names(self):
+        return (
+            ['attendee'] +
+            super(SprintAttendanceRegisterView, self).field_names)
 
     @property
     def initial_values(self):
@@ -172,6 +184,6 @@
     @action(_('Register'), name='register')
     def register_action(self, action, data):
         time_starts, time_ends = self.getDates(data)
-        is_physical = data['is_physical']
+        is_physical = self.context.is_physical and data['is_physical']
         self.context.attend(
             data['attendee'], time_starts, time_ends, is_physical)

=== modified file 'lib/lp/blueprints/interfaces/sprint.py'
--- lib/lp/blueprints/interfaces/sprint.py	2014-11-17 18:36:16 +0000
+++ lib/lp/blueprints/interfaces/sprint.py	2014-11-24 13:07:07 +0000
@@ -21,6 +21,7 @@
     Interface,
     )
 from zope.schema import (
+    Bool,
     Choice,
     Datetime,
     Int,
@@ -126,6 +127,9 @@
         title=_('Finishing Date and Time'), required=True)
     datecreated = Datetime(
         title=_('Date Created'), required=True, readonly=True)
+    is_physical = Bool(
+        title=_("Is the sprint being held in a physical location?"),
+        required=True, readonly=False, default=True)
 
     # joins
     attendees = Attribute('The set of attendees at this sprint.')

=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py	2013-08-19 06:43:04 +0000
+++ lib/lp/blueprints/model/sprint.py	2014-11-24 13:07:07 +0000
@@ -10,6 +10,7 @@
 
 
 from sqlobject import (
+    BoolCol,
     ForeignKey,
     StringCol,
     )
@@ -90,6 +91,7 @@
     time_zone = StringCol(notNull=True)
     time_starts = UtcDateTimeCol(notNull=True)
     time_ends = UtcDateTimeCol(notNull=True)
+    is_physical = BoolCol(notNull=True, default=True)
 
     # attributes
 
@@ -253,7 +255,7 @@
             attendance = SprintAttendance(sprint=self, attendee=person)
         attendance.time_starts = time_starts
         attendance.time_ends = time_ends
-        attendance.is_physical = is_physical
+        attendance._is_physical = is_physical
         return attendance
 
     def removeAttendance(self, person):
@@ -321,13 +323,13 @@
 
     def new(self, owner, name, title, time_zone, time_starts, time_ends,
             summary, address=None, driver=None, home_page=None,
-            mugshot=None, logo=None, icon=None):
+            mugshot=None, logo=None, icon=None, is_physical=True):
         """See `ISprintSet`."""
         return Sprint(owner=owner, name=name, title=title,
             time_zone=time_zone, time_starts=time_starts,
             time_ends=time_ends, summary=summary, driver=driver,
             home_page=home_page, mugshot=mugshot, icon=icon,
-            logo=logo, address=address)
+            logo=logo, address=address, is_physical=is_physical)
 
 
 class HasSprintsMixin:

=== modified file 'lib/lp/blueprints/model/sprintattendance.py'
--- lib/lp/blueprints/model/sprintattendance.py	2013-01-07 02:40:55 +0000
+++ lib/lp/blueprints/model/sprintattendance.py	2014-11-24 13:07:07 +0000
@@ -35,8 +35,12 @@
 
     time_starts = UtcDateTimeCol(notNull=True)
     time_ends = UtcDateTimeCol(notNull=True)
-    is_physical = Bool(default=True)
+    _is_physical = Bool(name='is_physical', default=True)
 
     def __init__(self, sprint, attendee):
         self.sprint = sprint
         self.attendee = attendee
+
+    @property
+    def is_physical(self):
+        return self.sprint.is_physical and self._is_physical

=== modified file 'lib/lp/blueprints/stories/sprints/xx-sprints.txt'
--- lib/lp/blueprints/stories/sprints/xx-sprints.txt	2014-02-19 02:11:16 +0000
+++ lib/lp/blueprints/stories/sprints/xx-sprints.txt	2014-11-24 13:07:07 +0000
@@ -118,6 +118,9 @@
     ...     'Starting Date and Time').value = '10 Oct 2006 09:15 '
     >>> user_browser.getControl(
     ...     'Finishing Date and Time').value = '13 Oct 2006 16:00'
+    >>> user_browser.getControl(
+    ...     'Is the sprint being held in a physical '
+    ...     'location?').selected = False
     >>> user_browser.getControl('Add Sprint').click()
 
     >>> user_browser.url
@@ -253,30 +256,31 @@
 
 We should be able to see the workload of a sprint:
 
-  >>> anon_browser.open('http://launchpad.dev/sprints/ubz/+assignments')
-  >>> print anon_browser.title
-  Assignments : Blueprints : Ubuntu Below Zero : Meetings
+    >>> anon_browser.open('http://launchpad.dev/sprints/ubz/+assignments')
+    >>> print anon_browser.title
+    Assignments : Blueprints : Ubuntu Below Zero : Meetings
 
 We should be able to see the spec assignment table of a sprint:
 
-  >>> mainarea = find_main_content(anon_browser.contents)
-  >>> for header in mainarea.findAll('th'):
-  ...     print header.renderContents()
-  Priority
-  Name
-  Definition
-  Delivery
-  Assignee
-  Drafter
-  Approver
+    >>> mainarea = find_main_content(anon_browser.contents)
+    >>> for header in mainarea.findAll('th'):
+    ...     print header.renderContents()
+    Priority
+    Name
+    Definition
+    Delivery
+    Assignee
+    Drafter
+    Approver
 
 And we should be able to see the workload page of a sprint even when there's
 no spec assigned to people.
 
-  >>> anon_browser.open('http://launchpad.dev/sprints/ltsponsteroids/+assignments')
-  >>> notice = find_tag_by_id(anon_browser.contents, 'no-blueprints')
-  >>> print extract_text(notice)
-  There are no open blueprints.
+    >>> anon_browser.open(
+    ...     'http://launchpad.dev/sprints/ltsponsteroids/+assignments')
+    >>> notice = find_tag_by_id(anon_browser.contents, 'no-blueprints')
+    >>> print extract_text(notice)
+    There are no open blueprints.
 
 
 Sprint Registration
@@ -309,6 +313,9 @@
     >>> browser.getControl('To').value
     '2006-02-12 17:00'
 
+    >>> browser.getControl(name='field.is_physical').value
+    ['yes']
+
 We accept a starting date up to one day before the sprint starts (which
 we will map to starting at the start of the sprint), and a departure
 date up to one day after the sprint ends.
@@ -403,6 +410,9 @@
     >>> browser.getControl('To').value
     '2006-02-12 17:00'
 
+    >>> browser.getControl(name='field.is_physical').value
+    ['yes']
+
 Sample Person can set a specific start and end time for participation,
 and specify that he is registering Carlos.
 
@@ -422,6 +432,7 @@
 
     >>> browser.getControl('Attendee').value = (
     ...     'guilherme.salgado@xxxxxxxxxxxxx')
+    >>> browser.getControl(name='field.is_physical').value = ['no']
     >>> browser.getControl('Register').click()
 
 And verifies that Carlos and Salgado are now listed:
@@ -461,14 +472,23 @@
     >>> carlos_browser = setupBrowser(auth='Basic carlos@xxxxxxxxxxxxx:test')
     >>> carlos_browser.open('http://launchpad.dev/sprints/ubz')
     >>> carlos_browser.getLink('Export attendees to CSV').click()
-    >>> print browser.headers['content-type']
+    >>> print carlos_browser.headers['content-type']
     text/csv
 
     >>> admin_browser.open('http://launchpad.dev/sprints/ubz')
     >>> admin_browser.getLink('Export attendees to CSV').click()
-    >>> print browser.headers['content-type']
+    >>> print admin_browser.headers['content-type']
     text/csv
 
+The resulting CSV file lists physical attendance correctly.
+
+    >>> import csv
+    >>> from StringIO import StringIO
+    >>> ubz_csv = list(csv.DictReader(StringIO(browser.contents)))
+    >>> [(row["Launchpad username"], row["Physically present"])
+    ...     for row in ubz_csv]
+    [('carlos', 'True'), ('salgado', 'False'), ('name12', 'True')]
+
 Unregistered and anonymous users cannot access the CSV report.
 
     >>> user_browser.open('http://launchpad.dev/sprints/ubz')
@@ -482,4 +502,20 @@
     ...
     Unauthorized:...
 
-
+Registering somebody for a remote-only sprint doesn't offer the choice of
+physical or remote attendance, and the CSV report always reports such people
+as attending remotely.
+
+    >>> browser.open('http://launchpad.dev/sprints/ltsponsteroids')
+    >>> browser.getLink('Register yourself').click()
+    >>> browser.getControl(name='field.is_physical')
+    Traceback (most recent call last):
+    ...
+    LookupError:...
+    >>> browser.getControl('Register').click()
+
+    >>> browser.getLink('Export attendees to CSV').click()
+    >>> ltsp_csv = list(csv.DictReader(StringIO(browser.contents)))
+    >>> [(row["Launchpad username"], row["Physically present"])
+    ...     for row in ltsp_csv]
+    [('name12', 'False')]


Follow ups