Merge lp:~linaro-infrastructure/launchpad/workitems-widget into lp:launchpad
| Status: | Merged | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Approved by: | Curtis Hovey on 2012-03-07 | ||||||||
| Approved revision: | no longer in the source branch. | ||||||||
| Merged at revision: | 14942 | ||||||||
| Proposed branch: | lp:~linaro-infrastructure/launchpad/workitems-widget | ||||||||
| Merge into: | lp:launchpad | ||||||||
| Diff against target: |
934 lines (+669/-6) 12 files modified
lib/lp/blueprints/adapters.py (+3/-1) lib/lp/blueprints/browser/configure.zcml (+6/-0) lib/lp/blueprints/browser/specification.py (+21/-1) lib/lp/blueprints/configure.zcml (+2/-1) lib/lp/blueprints/help/workitems-help.html (+50/-0) lib/lp/blueprints/interfaces/specification.py (+19/-0) lib/lp/blueprints/model/specification.py (+40/-2) lib/lp/blueprints/model/tests/test_specification.py (+117/-0) lib/lp/blueprints/templates/specification-index.pt (+26/-1) lib/lp/blueprints/tests/test_webservice.py (+6/-0) lib/lp/services/fields/__init__.py (+102/-0) lib/lp/services/fields/tests/test_fields.py (+277/-0) |
||||||||
| To merge this branch: | bzr merge lp:~linaro-infrastructure/launchpad/workitems-widget | ||||||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| j.c.sackett (community) | 2012-02-29 | Approve on 2012-03-08 | |
| Richard Harding (community) | code* | 2012-02-27 | Needs Fixing on 2012-03-02 |
| huwshimi | ui | 2012-02-28 | Pending |
|
Review via email:
|
|||
Commit Message
[r=jcsackett,
Description of the Change
Hi,
This branch adds a text area for editing Work Items, similar to the blueprint Whiteboard. The input will be parsed and verified to conform to the format assumed today by status.ubuntu.com and status.linaro.org. Additionally some validation is done to ensure that valid lp accounts are used as assignees as well as milestones that are valid targets for the specification.
The format of this text area is:
--
This is blocked on something: BLOCKED
This is already done: DONE
[bob] Something Bob should do: TODO
Work items for ubuntu-12.04:
This will be worked on later: TODO
--
Thanks,
Mattias
| Guilherme Salgado (salgado) wrote : | # |
| James Tunnicliffe (dooferlad) wrote : | # |
I think the whiteboard being first is nice, but as you say, it may cause people to miss the new box. How about a highlighted message to users about work items having moved that shows up above the whiteboard? It could be designed so that the user can hide the message and it sets a cookie so they don't see it again. The message can be deleted from the UI once sufficient time has passed that most users are using the two boxes in the new way.
| Richard Harding (rharding) wrote : | # |
Thanks, this looks ok. I'd like to suggest a couple of tweaks, but they're mostly optional preference kinds of things.
I think it'd be better to move the compiled regex to module level constants so that it only gets compiled once when the app starts. They're not going to change after that.
Is there any reason you did regex matching for the parse method, but break the string apart manually in the parseLine method? It seems you'd be able to have two possible regexes (with and without [assignee]) to toss at it and you could grab out the title, status, assignee via the regex groups?
The nice thing about moving those outside like that and as regexes is that it's be easy to toss a quick run through of strings in tests to make for a very clear/quick view of potential issues matching and easily updated in the future as someone thinks of something that breaks it.
I'd also like to see the parseLine broken into parsing and validating. Again, it makes it easier to test the parser and the validation bits separately. Right now validation errors can come from parseLine, getAssignee, getMilestone.
| Mattias Backman (mabac) wrote : | # |
> Thanks, this looks ok. I'd like to suggest a couple of tweaks, but they're
> mostly optional preference kinds of things.
>
> I think it'd be better to move the compiled regex to module level constants so
> that it only gets compiled once when the app starts. They're not going to
> change after that.
Absolutely, done.
>
> Is there any reason you did regex matching for the parse method, but break the
> string apart manually in the parseLine method? It seems you'd be able to have
The main reason, apart from snipping the code from work-items-tracker was to provide detailed error messages if parsing failed.
> two possible regexes (with and without [assignee]) to toss at it and you could
> grab out the title, status, assignee via the regex groups?
I tried that and still kept the ability to give detailed feedback. The code got cleaner but the re is pretty unreadable. I believe that there are enough tests to catch if someone ever messes it up though.
>
> The nice thing about moving those outside like that and as regexes is that
> it's be easy to toss a quick run through of strings in tests to make for a
> very clear/quick view of potential issues matching and easily updated in the
> future as someone thinks of something that breaks it.
I didn't change the tests to use the re directly, but I can do that too if you'd like that.
>
> I'd also like to see the parseLine broken into parsing and validating. Again,
> it makes it easier to test the parser and the validation bits separately.
> Right now validation errors can come from parseLine, getAssignee,
> getMilestone.
I split out the status validation to getStatus() so parseLine is now only re matching and checking the title, all parsing. I still raise LaunchpadValida
| j.c.sackett (jcsackett) wrote : | # |
This looks pretty good. I have a few questions and nitpicks in the diff below.
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -179,6 +184,98 @@
> self.assertEqua
> self.assertEqua
>
> + def test_workitems_
> + spec = self.factory.
> + self.assertEqua
> +
> + def test_workitems_
> + work_item = self.factory.
> + self.assertEqua
> +
> + def test_workitems_
> + work_item = self.factory.
> + self.assertWork
> +
> + def test_workitems_
> + work_item1 = self.factory.
> + status=
> + work_item2 = self.factory.
> + specification=
> + status=
> + work_item3 = self.factory.
> + specification=
> + status=
> + work_item4 = self.factory.
> + specification=
> + status=
> + work_item5 = self.factory.
> + specification=
> + status=
> + work_items = [work_item1, work_item2, work_item3, work_item4, work_item5]
> + self.assertWork
This and other tests would read clearer if you first made a spec with
factory.
factory.
> + def test_workitems_
> + spec = self.factory.
> + milestone = self.factory.
> + login_person(
> + work_item = spec.newWorkItem(
> + title=u'
> + status=
> + milestone=
> + expected_wi_text = ("Work items for %s:\n"
> + "%s: TODO" % \
> + (milestone.name, work_item.title))
> + self.assertEqua
> +
> + def test_workitems_
> + spec = self.factory.
> + milestone = self.factory.
> + login_person(
> + work_item1 = spec.newWorkItem(
> + ...
| Mattias Backman (mabac) wrote : | # |
> This looks pretty good. I have a few questions and nitpicks in the diff below.
>
>
> > === modified file 'lib/lp/
> > --- lib/lp/
> 04:24:19 +0000
> > +++ lib/lp/
> 13:48:22 +0000
> > @@ -179,6 +184,98 @@
> > self.assertEqua
> > self.assertEqua
> >
> > + def test_workitems_
> > + spec = self.factory.
> > + self.assertEqua
> > +
> > + def test_workitems_
> > + work_item = self.factory.
> > + self.assertEqua
> > +
> > + def test_workitems_
> > + work_item = self.factory.
> > + self.assertWork
> [work_item])
> > +
> > + def test_workitems_
> > + work_item1 = self.factory.
> > + status=
> > + work_item2 = self.factory.
> > + specification=
> > + status=
> > + work_item3 = self.factory.
> > + specification=
> > + status=
> > + work_item4 = self.factory.
> > + specification=
> > + status=
> > + work_item5 = self.factory.
> > + specification=
> > + status=
> > + work_items = [work_item1, work_item2, work_item3, work_item4,
> work_item5]
> > + self.assertWork
> work_items)
>
> This and other tests would read clearer if you first made a spec with
> factory.
> factory.
I've changed that, hope I didn't miss any tests that can be improved.
>
> > + def test_workitems_
> > + spec = self.factory.
> > + milestone = self.factory.
> > + login_person(
> > + work_item = spec.newWorkItem(
> > + title=u'
> > + status=
> > + milestone=
> > + expected_wi_text = ("Work items for %s:\n"
> > + "%s: TODO" % \
> > + (milestone.name, work_item.title))
> > + self.assertEqua
> > +
> > + def test_workitems_
> >...
| Guilherme Salgado (salgado) wrote : | # |
Great to see this is ready. Just wanted to note that we should wait until the migration script has been QAed before we land this. That way we can turn the script on as soon as this is deployed.
Another option would be to guard the new UI with the same feature flag used in the script.
| Guilherme Salgado (salgado) wrote : | # |
I've just noticed the widget we use for work-items-text doesn't display validation errors anywhere, so if the user enters unparseable/invalid data it will fail without giving the user any clue about what's wrong. This is a bug present in all text-editing widgets in Launchpad (e.g. the tags widget in a bug page will not tell you what's wrong if you enter a tag with upper-case letters), though, so it's something we'll need to fix in a separate branch.
| Guilherme Salgado (salgado) wrote : | # |
There's a bug report for that already: bug 415365
| huwshimi (huwshimi) wrote : | # |
From my cursory look into how people use the whiteboard there is usually some kind of information before people list the work items (but not on all). With the work items field first this switches that pattern around.
There is also a pattern which usually goes, information about this item, then related information. In this case we have Blueprint information, details/discussion of the Blueprint (whiteboard) and lastly work items. I think the order of work items and whiteboard in a sense depends on what kind of information the whiteboard is being used for (not helpful, I know).
Lastly we have importance of information. As there can potentially be a lot of information for both the whiteboard and work items we may have to prioritise the information that is most important for the user to be able to see upfront.
It would seem like the order should be whiteboard then work items, but with some clarification on the importance of work items and the way whiteboards are used then we might discover which order is best.
As James says a message about the new field for work items may help if work items are last. I would suggest showing that message when the whiteboard is in edit mode.
Hope that helps somewhat. Sorry I can't be more concrete with my answer.
| Mattias Backman (mabac) wrote : | # |
> From my cursory look into how people use the whiteboard there is usually some
> kind of information before people list the work items (but not on all). With
> the work items field first this switches that pattern around.
>
> There is also a pattern which usually goes, information about this item, then
> related information. In this case we have Blueprint information,
> details/discussion of the Blueprint (whiteboard) and lastly work items. I
> think the order of work items and whiteboard in a sense depends on what kind
> of information the whiteboard is being used for (not helpful, I know).
>
> Lastly we have importance of information. As there can potentially be a lot of
> information for both the whiteboard and work items we may have to prioritise
> the information that is most important for the user to be able to see upfront.
>
> It would seem like the order should be whiteboard then work items, but with
> some clarification on the importance of work items and the way whiteboards are
> used then we might discover which order is best.
>
> As James says a message about the new field for work items may help if work
> items are last. I would suggest showing that message when the whiteboard is in
> edit mode.
I have switched the order of the text fields so that the Whiteboard is on top. I also added an informational message which is hidden by default and is shown by some js when the editor is shown. When the editor is hidden, the message goes away.
>
> Hope that helps somewhat. Sorry I can't be more concrete with my answer.
| Richard Harding (rharding) wrote : | # |
Mattias, if we're going to add support in the widget for informational messages I think it would need to get through a UI review of how to display and such. We'd want to fully implement it as the error message is done. Since this is the first case that needs this, I'd almost rather tack it on at the moment.
You can implement your message in just this location with the following diff:
=== modified file 'lib/lp/
--- lib/lp/
+++ lib/lp/
@@ -325,7 +325,7 @@
</div>
<script type="text/
- LPJS.use('lp.anim', 'lp.ui', function(Y) {
+ LPJS.use('lp.anim', 'lp.ui', 'node', 'widget' function(Y) {
Y.on('lp:context:implementation
var icon = Y.one('
@@ -366,6 +366,24 @@
});
+ // Watch for the whiteboard for edit mode so we can show/hide a
+ // message to the user to make sure not to put work items in there.
+ var whiteboard_node = Y.one('
+ var whiteboard = Y.Widget.
+ var notice_node = Y.Node.
+ notice_
+ notice_
+ notice_
+ whiteboard.
+ // If we're visible, show the message
+ if (ev.newVal) {
+ var par = whiteboard_
+ par.insertBefor
+ } else {
+ // Otherwise we need to remove the node
+ notice_
+ }
+ });
});
</script>
For the long term, please submit a bug for adding proper support for informational messages to the inlineedit widget. We can look at the right way to support it from there.
| Mattias Backman (mabac) wrote : | # |
> Mattias, if we're going to add support in the widget for informational
> messages I think it would need to get through a UI review of how to display
> and such. We'd want to fully implement it as the error message is done. Since
> this is the first case that needs this, I'd almost rather tack it on at the
> moment.
>
> You can implement your message in just this location with the following diff:
Cool thanks! I used your code and just did a minor tweak which seems to work better when showing/hiding the editor multiple times.
>
>
> === modified file 'lib/lp/
> --- lib/lp/
> +0000
> +++ lib/lp/
> +0000
> @@ -325,7 +325,7 @@
> </div>
>
> <script type="text/
> - LPJS.use('lp.anim', 'lp.ui', function(Y) {
> + LPJS.use('lp.anim', 'lp.ui', 'node', 'widget' function(Y) {
>
> Y.on('lp:context:implementation
> var icon = Y.one('
> @@ -366,6 +366,24 @@
> window.
> });
>
> + // Watch for the whiteboard for edit mode so we can show/hide a
> + // message to the user to make sure not to put work items in there.
> + var whiteboard_node = Y.one('
> + var whiteboard = Y.Widget.
> + var notice_node = Y.Node.
> + notice_
> + notice_
> + notice_
> separate Work Items input field below.');
> + whiteboard.
> + // If we're visible, show the message
> + if (ev.newVal) {
> + var par = whiteboard_
> + par.insertBefor
> + } else {
> + // Otherwise we need to remove the node
> + notice_
> + }
> + });
> });
> </script>
>
>
> For the long term, please submit a bug for adding proper support for
> informational messages to the inlineedit widget. We can look at the right way
> to support it from there.
Sure, this is the bug: https:/
| j.c.sackett (jcsackett) wrote : | # |
These changes look fine to me. Thanks for factoring all of these notes into your branch, Mattias.
| Guilherme Salgado (salgado) wrote : | # |
Matsubara is now hammering this to see if he finds any bugs we haven't found yet. We also need to work out a plan to notify people about the BPs we won't be able to migrate and an announcement for the UI changes. Once that's done we can land this.
In the meantime I'm merging lp:~salgado/launchpad/workitems-widget-help-popup in here because here's where it belongs (and it has been approved already)
| Mattias Backman (mabac) wrote : | # |
I have added a few revisions to fix a bug; we actually had changed the format a little by not inserting a milestone header for the default milestone (which is inherited from the spec).
14859 is approved elsewhere.
14861-14863 adds the default milestone header.
| j.c.sackett (jcsackett) wrote : | # |
A cleaner diff of what the last few revisions contain (removing updates from devel and the popup branch) can be found at http://

I'm asking Huw to review this mostly to get feedback on the position of the new field on the page... should it be before the whiteboard or after it? Having it before makes it less likely for people to miss it and enter work-items on the whiteboard, but there may be reasons to have it after the whiteboard as well.