Merge lp://qastaging/~sinzui/launchpad/sprint-is-physical into lp://qastaging/launchpad/db-devel

Proposed by Curtis Hovey
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://qastaging/~sinzui/launchpad/sprint-is-physical
Merge into: lp://qastaging/launchpad/db-devel
Diff against target: 33 lines
2 files modified
database/schema/comments.sql (+3/-0)
database/schema/patch-2207-04-0.sql (+13/-0)
To merge this branch: bzr merge lp://qastaging/~sinzui/launchpad/sprint-is-physical
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) release-critical Disapprove
Brad Crittenden (community) release-critical Disapprove
Stuart Bishop (community) db Approve
Jonathan Lange (community) db Approve
Canonical Launchpad Engineering db Pending
Review via email: mp+12026@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

This is my branch to add a is_physical flag for SprintAttendance. This
branch just adds the column tot he DB so that edge users to test this
feature as it is developed in October.

    lp:~sinzui/launchpad/sprint-in-person
    Diff size: 496 (thank you make newsampledata)
    Launchpad bug: https://bugs.launchpad.net/bugs/430945
    Test command: make schema
    Pre-implementation: no one
    Target release: 3.1.0

= Add a is_physical flag for SprintAttendance =

In order to better plan UDS we'd like the sprint/meeting feature to have a
checkbox so we can tell when an attendee is physically attending an event or
participating remotely. Something like:

    [ ] I plan to physically attend this event at $location.

Curtis adds "The background of this issue reminds me a request to modify the
person profile to have a t-shirt size field for sprints."

ADDENDUM 1

There is no bug for the t-shirt, but I have been asked it often enough from
HR that I offer to add this on my own time. If you feel the column is not
worthy of Launchpad, my feelings will not be hurt.

ADDENDUM 2

The DB has SprintAttendance.date_created, but the interface and model
do not know about this data. I suspect there was a place to capture this
information. I imagine the migration script would use the start_date of
the sprint as the date created for the attendance. This is not important
to me so I have chosen to settle this matter later.

== Rules ==

    * Add is_physical to SprintAttendance
      The value is BOOLEAN
    * t_shirt_size text to Person.
      The interface will define an enum:
      UNKNOWN, SMALL, MEDIUM, LARGE, EXTRA_LARGE, EXTRA_EXTRA_LARGE

== Lint ==

Linting changed files:
  database/sampledata/current-dev.sql
  database/sampledata/current.sql
  database/schema/comments.sql
  database/schema/patch-2109-97-0.sql

== Implementation ==

    * database/sampledata/current-dev.sql
      * Updated using make newsampledata.
    * database/sampledata/current.sql
      * Updated using make newsampledata.
    * database/schema/comments.sql
      * Added comments for the two columns I am adding.
      * Added missing comments for the sprint and attendee
    * database/schema/patch-2109-97-0.sql
      * Add the SprintAttendance.is_physical and Person.t_shirt_size columns.

Revision history for this message
Stuart Bishop (stub) wrote :

It is incorrect to have t-shirt size on person, as sizes change depending on location. For example, if I'm registered for sprints in the USA and Asia I'll need a Large for the US sprint and a X-Large for the Asian one. Also, You most likely need a flag on the Sprint table, specifying if t-shirts may be offered - no point asking people for their t-shirt sizes if there will be none.

review: Needs Fixing (db)
Revision history for this message
Stuart Bishop (stub) wrote :

> ADDENDUM 2
>
> The DB has SprintAttendance.date_created, but the interface and model
> do not know about this data. I suspect there was a place to capture this
> information. I imagine the migration script would use the start_date of
> the sprint as the date created for the attendance. This is not important
> to me so I have chosen to settle this matter later.

SprintAttendance.date_created seems fine - it is the creation date of the record in the database, similar to many other date_created fields. I don't see any particular need for the Launchpad application itself to know or care about this information; we just capture it in case we need it and maybe to draw pretty graphs from.

Revision history for this message
Jonathan Lange (jml) wrote :

I agree with stub regarding Person.t_shirt_size. Perhaps SprintAttendance would be the least bad place for this. The form code could then use the person's last given t-shirt size as a best-guess default.

It makes me wonder though, what else do sprint organizers need to know? I can't see any field in the database for storing a person's dietary requirements, for example. What are we actually going to do with this data?

The 'is_physical' flag makes sense. 'date_created' might have some actual uses (e.g. a prize to the first person who signs up), but I don't think that it's worth worrying about right now.

review: Needs Fixing (db)
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Jono and Stuart.

Thanks for the review. Stuarts argument against the t-shirt is very
convincing. I dropped the column. I still hope to land this for 3.0 so
that UDS planning can use the feature on edge.

On Fri, 2009-09-18 at 05:19 +0000, Jonathan Lange wrote:
> Review: Needs Fixing db
> I agree with stub regarding Person.t_shirt_size. Perhaps
> SprintAttendance would be the least bad place for this. The form code
> could then use the person's last given t-shirt size as a best-guess
> default.
>
> It makes me wonder though, what else do sprint organizers need to
> know? I can't see any field in the database for storing a person's
> dietary requirements, for example. What are we actually going to do
> with this data?
>
> The 'is_physical' flag makes sense. 'date_created' might have some
> actual uses (e.g. a prize to the first person who signs up), but I
> don't think that it's worth worrying about right now.

This is the complete patch file:

{{{

-- Copyright 2009 Canonical Ltd. This software is licensed under the
-- GNU Affero General Public License version 3 (see the file LICENSE).

SET client_min_messages=ERROR;

-- Add a column to indicate that the attendee is physically attending
-- the sprint.
ALTER TABLE SprintAttendance
  ADD COLUMN is_physical BOOLEAN NOT NULL DEFAULT FALSE;

INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 97, 0);

}}}

This is the incremental diff sans sampledata changes.

{{{

=== modified file 'database/schema/comments.sql'
--- a/database/schema/comments.sql 2009-09-18 03:18:28 +0000
+++ b/database/schema/comments.sql 2009-09-18 14:28:55 +0000
@@ -1150,7 +1150,6 @@
 COMMENT ON COLUMN Person.mailing_list_receive_duplicates IS 'True means the user wants to receive list copies of messages on which they are explicitly named as a recipient.';
 COMMENT ON COLUMN Person.visibility IS 'person.PersonVisibility enumeration which can be set to Public, Public with Private Membership, or Private.';
 COMMENT ON COLUMN Person.verbose_bugnotifications IS 'If true, all bugnotifications sent to this Person will include the bug description.';
-COMMENT ON COLUMN Person.t_shirt_size IS 'The t-shirt size the person prefers when attending a sprint.';

 COMMENT ON VIEW ValidPersonCache IS 'A materialized view listing the Person.ids of all valid people (but not teams).';

=== renamed file 'database/schema/patch-2109-97-0.sql' => 'database/schema/patch-2207-97-0.sql'
--- a/database/schema/patch-2109-97-0.sql 2009-09-18 02:27:28 +0000
+++ b/database/schema/patch-2207-97-0.sql 2009-09-18 14:28:35 +0000
@@ -10,10 +10,4 @@
   ADD COLUMN is_physical BOOLEAN NOT NULL DEFAULT FALSE;

--- Add a column to to specify the user's t-shirt-size for sprints.
--- 0 UNKNOWN, 1 SMALL, 2 MEDIUM, 3 LARGE, 4 EXTRA_LARGE, 5 EXTRA_EXTRA_LARGE
-ALTER TABLE Person
- ADD COLUMN t_shirt_size INT NOT NULL DEFAULT 0;
-
-
-INSERT INTO LaunchpadDatabaseRevision VALUES (2109, 97, 0);
+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 97, 0);

}}}

Revision history for this message
Jonathan Lange (jml) wrote :

I'm OK with this patch, pending stub's approval.

review: Approve (db)
Revision history for this message
Stuart Bishop (stub) wrote :

Approved as patch-2207-04-0.sql

I'm not that hot on the name of the new column, but it is good enough for now.

review: Approve (db)
Revision history for this message
Brad Crittenden (bac) wrote :

A db patch cannot be included in a re-roll unless it is critical to the on-going operation of Launchpad.

review: Disapprove (release-critical)
Revision history for this message
Francis J. Lacoste (flacoste) wrote :

We are not including DB patches in the re-roll.

review: Disapprove (release-critical)
Revision history for this message
Francis J. Lacoste (flacoste) wrote :

If this is really critical to UDS success, we can ask Stuart to manually apply the DB patch.

Revision history for this message
Stuart Bishop (stub) wrote :

> If this is really critical to UDS success, we can ask Stuart to manually apply
> the DB patch.

I can't add new columns without downtime and/or switching on read-only mode due to replication issues.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'database/schema/comments.sql'
2--- database/schema/comments.sql 2009-08-30 01:41:38 +0000
3+++ database/schema/comments.sql 2009-09-29 02:05:32 +0000
4@@ -763,8 +763,11 @@
5
6 -- SprintAttendance
7 COMMENT ON TABLE SprintAttendance IS 'The record that someone will be attending a particular sprint or meeting.';
8+COMMENT ON COLUMN SprintAttendance.attendee IS 'The person attending the sprint.';
9+COMMENT ON COLUMN SprintAttendance.sprint IS 'The sprint the person is attending.';
10 COMMENT ON COLUMN SprintAttendance.time_starts IS 'The time from which the person will be available to participate in meetings at the sprint.';
11 COMMENT ON COLUMN SprintAttendance.time_ends IS 'The time of departure from the sprint or conference - this is the last time at which the person is available for meetings during the sprint.';
12+COMMENT ON COLUMN SprintAttendance.is_physical IS 'Is the person physically attending the sprint';
13
14
15 -- SprintSpecification
16
17=== added file 'database/schema/patch-2207-04-0.sql'
18--- database/schema/patch-2207-04-0.sql 1970-01-01 00:00:00 +0000
19+++ database/schema/patch-2207-04-0.sql 2009-09-29 02:05:32 +0000
20@@ -0,0 +1,13 @@
21+-- Copyright 2009 Canonical Ltd. This software is licensed under the
22+-- GNU Affero General Public License version 3 (see the file LICENSE).
23+
24+SET client_min_messages=ERROR;
25+
26+
27+-- Add a column to indicate that the attendee is physically attending
28+-- the sprint.
29+ALTER TABLE SprintAttendance
30+ ADD COLUMN is_physical BOOLEAN NOT NULL DEFAULT FALSE;
31+
32+
33+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 04, 0);

Subscribers

People subscribed via source and target branches

to status/vote changes: