Merge lp://staging/~henninge/launchpad/bug-435712 into lp://staging/launchpad/db-devel

Proposed by Henning Eggers
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://staging/~henninge/launchpad/bug-435712
Merge into: lp://staging/launchpad/db-devel
Diff against target: 162 lines
9 files modified
lib/lp/answers/browser/question.py (+4/-0)
lib/lp/answers/stories/question-add.txt (+5/-4)
lib/lp/answers/stories/question-confirm-url.txt (+1/-1)
lib/lp/answers/stories/question-edit.txt (+1/-1)
lib/lp/answers/stories/question-reject-and-change-status.txt (+1/-1)
lib/lp/answers/stories/question-workflow.txt (+1/-1)
lib/lp/answers/stories/this-is-a-faq.txt (+1/-1)
lib/lp/answers/templates/question-index.pt (+8/-14)
lib/lp/bugs/stories/bugs/xx-bug-create-question.txt (+2/-2)
To merge this branch: bzr merge lp://staging/~henninge/launchpad/bug-435712
Reviewer Review Type Date Requested Status
Brad Crittenden (community) release-critical Approve
Paul Hummer (community) ui Approve
Michael Nelson (community) code Approve
Canonical Launchpad Engineering code Pending
Review via email: mp+12347@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

= Summary =

Fixes bug 435712 by moving the details about a question to the "registering" slot, like it is with bugs and branches. The language information was simply moved to the main body.

== Screenshots ==

Before and after:
http://people.canonical.com/~henninge/screenshots/answers-bug-435712-before.png
http://people.canonical.com/~henninge/screenshots/answers-bug-435712-after.pn

== Test command ==

bin/test -vvt question -t this-is-a-faq

== Demo/QA ==

Go to any question and verify that is looks like the "after" screenshot avbove.

Revision history for this message
Henning Eggers (henninge) wrote :
Revision history for this message
Michael Nelson (michael.nelson) wrote :
Download full text (8.7 KiB)

Hi Henning!

Thanks for getting in there and getting this page a bit more consistent!

I'm setting this to code-approved - as I'm guessing you've already got
it running through ec2test, but I'd like someone else to second the
ui as I've got some questions about the usage of the registration
slot.

> = Summary =
>
> Fixes bug 435712 by moving the details about a question to the "registering"
> slot, like it is with bugs and branches.

Right - I hadn't realised that we were putting more than the actual
registration info into that slot - but can see that it's already done
on the bugs page. That is, I hadn't realised we were going to communicate
status and/or repeat the context (bug XXX or question xxx) in that slot.

I personally prefer the branches page, where it is *just* the registration
info:

Created by Michael Nelson on 2009-09-23 and last modified on 2009-09-23

rather than what's on the bugs page:

Bug #201001 reported by Celso Providelo on 2008-03-11

which I reckon ought to be:

Reported by Celso Providelo on 2008-03-11

but I haven't been involved in any of the discussions - so perhaps there's
already been a decision there? I just think that yet another reference
to 'Bug #xxxxx' or '...question #xxxx' is unnecessary (and getting
noisy). What are your thoughts? The status could go below with the language
details in a dl (like on other 3-0 pages - the object details).

A note regarding the status - I don't think it's part of this branch,
but I think it should be "Answered question" rather than "Answered Question".
See: https://dev.launchpad.net/UserInterfaceWording#Capitalization

Ah, I just noticed that there's no leaf breadcrumb 'Answered question #xxx'.
But as you said in irc, that's a separate bug.

> The language information was simply
> moved to the main body.
>
> == Screenshots ==
>
> Before and after:
> http://people.canonical.com/~henninge/screenshots/answers-
> bug-435712-before.png
> http://people.canonical.com/~henninge/screenshots/answers-bug-435712-after.pn
>

Thanks for the screenshots!

> == Test command ==
>
> bin/test -vvt question -t this-is-a-faq
>
> == Demo/QA ==
>
> Go to any question and verify that is looks like the "after" screenshot
> avbove.

Great.

> === modified file 'lib/lp/answers/browser/question.py'
> --- lib/lp/answers/browser/question.py 2009-09-22 15:43:52 +0000
> +++ lib/lp/answers/browser/question.py 2009-09-24 14:18:35 +0000
> @@ -755,6 +755,10 @@
> initial_focus_widget = None
>
> @property
> + def label(self):
> + return self.context.title
> +
> + @property
> def page_title(self):
> return smartquote('%s question #%d: "%s"') % (
> self.context.target.displayname,
>
> === modified file 'lib/lp/answers/stories/question-add.txt'
> --- lib/lp/answers/stories/question-add.txt 2009-09-23 14:40:53 +0000
> +++ lib/lp/answers/stories/question-add.txt 2009-09-24 14:18:35 +0000
> @@ -157,9 +157,9 @@
> >>> user_browser.title
> 'Questions : \xe2\x80\x9cmozilla-firefox\xe2\x80\x9d package : Ubuntu'
>
> - >>> contents = find_main_content(user_browser.contents)
> - >>> contents.findChild('div', id='question-details')
> + >>> prin...

Read more...

review: Approve (code)
Revision history for this message
Paul Hummer (rockstar) wrote :

Thanks for doing this!

review: Approve (ui)
Revision history for this message
Henning Eggers (henninge) wrote :
Download full text (5.3 KiB)

Hi Michael,
thanks for your review. Let me just quickly comment on your suggestions
in the code.

Am 24.09.2009 17:11, Michael Nelson schrieb:
>> === modified file 'lib/lp/answers/stories/question-add.txt'
>> --- lib/lp/answers/stories/question-add.txt 2009-09-23 14:40:53 +0000
>> +++ lib/lp/answers/stories/question-add.txt 2009-09-24 14:18:35 +0000
>> @@ -157,9 +157,9 @@
>> >>> user_browser.title
>> 'Questions : \xe2\x80\x9cmozilla-firefox\xe2\x80\x9d package : Ubuntu'
>>
>> - >>> contents = find_main_content(user_browser.contents)
>> - >>> contents.findChild('div', id='question-details')
>> + >>> print find_tag_by_id(user_browser.contents, 'registration')
>> <div...>Open<... Question #..., asked...by...No Privileges Person...
>
> Any reason for not using extract_text here? Might be nicer to read...

No, none. It actually crossed my mind, too. Done.

>
>> + >>> contents = find_main_content(user_browser.contents)
>> >>> contents.find('div', 'report')
>
> ... and here: print extract_text(contents.find('div', 'report'))

Did that, too.

>
>> <div class="report"><p>I use Ubuntu on AMD64 ...</div>
>>
>>
>> === modified file 'lib/lp/answers/stories/question-confirm-url.txt'
>> --- lib/lp/answers/stories/question-confirm-url.txt 2009-08-25 21:30:02 +0000
>> +++ lib/lp/answers/stories/question-confirm-url.txt 2009-09-24 14:18:35 +0000
>> @@ -96,7 +96,7 @@
>> This adds his comment to the question and mark it as 'Solved.'
>>
>> >>> print extract_text(
>> - ... find_tag_by_id(owner_browser.contents, 'question-details'))
>> + ... find_tag_by_id(owner_browser.contents, 'registration'))
>> Solved ...
>
> Ah, so it's also communicating the status... - I personally am not
> convinced that it belongs there - but maybe you've had discussions
> with others about it?
>
> I think it should simply be:
>
> Asked on 2006-01-01 by Guilherme Salgado (and potentially last updated -
> not sure if that's in the db?)

You are probably right but that would include more work on the whole
page which would break more tests ... Too much for too little time ;-)

>
>> >>> print find_tags_by_class(
>> ... owner_browser.contents, 'boardCommentBody')[-1].renderContents()
>>
>> === modified file 'lib/lp/answers/stories/question-reject-and-change-status.txt'
>> --- lib/lp/answers/stories/question-reject-and-change-status.txt 2009-08-25 21:30:02 +0000
>> +++ lib/lp/answers/stories/question-reject-and-change-status.txt 2009-09-24 14:18:35 +0000
>> @@ -52,7 +52,7 @@
>>
>> >>> def print_question_status(browser):
>> ... print extract_text(
>> - ... find_tag_by_id(browser.contents, 'question-details'))
>> + ... find_tag_by_id(browser.contents, 'registration'))
>
> Hmm... perhaps it's worth moving this into something like
> l/c/lp/testing/pages.py? See what you think.

Haven't looked at that yet but usually I am all for this kind of stuff.
But ...

[...]
>> === modified file 'lib/lp/answers/templates/question-index.pt'
>> --- lib/lp/answers/templates/question-index.pt 2009-09-03 20:22:39 +0000
>> +++ lib/lp/answers/templates/question-index.pt 2009-09-24 14:18:3...

Read more...

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

This patch is approved for the re-roll. It is not 'release-critical' but it is a safe fix and will help us achieve more consistency across apps.

Thanks for the fix.

review: Approve (release-critical)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Henning Eggers wrote:
> Am 24.09.2009 17:11, Michael Nelson schrieb:
>>
>> I think it should simply be:
>>
>> Asked on 2006-01-01 by Guilherme Salgado (and potentially last updated -
>> not sure if that's in the db?)
>
> You are probably right but that would include more work on the whole
> page which would break more tests ... Too much for too little time ;-)
>

Yeah - totally. Your changes are great - we can look at standardising
the use of the registration slot later :)

Great work for getting it landed!

- --
Michael
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkq7tYYACgkQGSan/irvan0VbgCcDtUfpxACW+bdW20SE9dr0eYq
MLYAn0ErEvvLZeLsIXctMfNmCZyPUU6O
=XVhv
-----END PGP SIGNATURE-----

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/answers/browser/question.py'
2--- lib/lp/answers/browser/question.py 2009-09-22 15:43:52 +0000
3+++ lib/lp/answers/browser/question.py 2009-09-24 17:29:11 +0000
4@@ -755,6 +755,10 @@
5 initial_focus_widget = None
6
7 @property
8+ def label(self):
9+ return self.context.title
10+
11+ @property
12 def page_title(self):
13 return smartquote('%s question #%d: "%s"') % (
14 self.context.target.displayname,
15
16=== modified file 'lib/lp/answers/stories/question-add.txt'
17--- lib/lp/answers/stories/question-add.txt 2009-09-23 14:40:53 +0000
18+++ lib/lp/answers/stories/question-add.txt 2009-09-24 17:29:11 +0000
19@@ -157,11 +157,12 @@
20 >>> user_browser.title
21 'Questions : \xe2\x80\x9cmozilla-firefox\xe2\x80\x9d package : Ubuntu'
22
23+ >>> print extract_text(
24+ ... find_tag_by_id(user_browser.contents, 'registration'))
25+ Open... Question #..., asked...by...No Privileges Person...
26 >>> contents = find_main_content(user_browser.contents)
27- >>> contents.findChild('div', id='question-details')
28- <div...>Open<... Question #..., asked...by...No Privileges Person...
29- >>> contents.find('div', 'report')
30- <div class="report"><p>I use Ubuntu on AMD64 ...</div>
31+ >>> print extract_text(contents.find('div', 'report'))
32+ I use Ubuntu on AMD64 ...
33
34 The user automatically got subscribed to this new question.
35
36
37=== modified file 'lib/lp/answers/stories/question-confirm-url.txt'
38--- lib/lp/answers/stories/question-confirm-url.txt 2009-08-25 21:30:02 +0000
39+++ lib/lp/answers/stories/question-confirm-url.txt 2009-09-24 17:29:11 +0000
40@@ -96,7 +96,7 @@
41 This adds his comment to the question and mark it as 'Solved.'
42
43 >>> print extract_text(
44- ... find_tag_by_id(owner_browser.contents, 'question-details'))
45+ ... find_tag_by_id(owner_browser.contents, 'registration'))
46 Solved ...
47 >>> print find_tags_by_class(
48 ... owner_browser.contents, 'boardCommentBody')[-1].renderContents()
49
50=== modified file 'lib/lp/answers/stories/question-edit.txt'
51--- lib/lp/answers/stories/question-edit.txt 2009-08-25 21:30:02 +0000
52+++ lib/lp/answers/stories/question-edit.txt 2009-09-24 17:29:11 +0000
53@@ -41,7 +41,7 @@
54
55 >>> def print_question_status(browser):
56 ... print extract_text(
57- ... find_tag_by_id(browser.contents, 'question-details'))
58+ ... find_tag_by_id(browser.contents, 'registration'))
59
60 >>> user_browser.open('http://launchpad.dev/ubuntu/+question/3')
61 >>> print_question_status(user_browser)
62
63=== modified file 'lib/lp/answers/stories/question-reject-and-change-status.txt'
64--- lib/lp/answers/stories/question-reject-and-change-status.txt 2009-08-25 21:30:02 +0000
65+++ lib/lp/answers/stories/question-reject-and-change-status.txt 2009-09-24 17:29:11 +0000
66@@ -52,7 +52,7 @@
67
68 >>> def print_question_status(browser):
69 ... print extract_text(
70- ... find_tag_by_id(browser.contents, 'question-details'))
71+ ... find_tag_by_id(browser.contents, 'registration'))
72
73 >>> print_question_status(admin_browser)
74 Invalid ...
75
76=== modified file 'lib/lp/answers/stories/question-workflow.txt'
77--- lib/lp/answers/stories/question-workflow.txt 2009-09-18 15:24:30 +0000
78+++ lib/lp/answers/stories/question-workflow.txt 2009-09-24 17:29:11 +0000
79@@ -15,7 +15,7 @@
80 # added and the status of the question.
81 >>> def find_request_status(contents):
82 ... print extract_text(
83- ... find_tag_by_id(contents, 'question-details'))
84+ ... find_tag_by_id(contents, 'registration'))
85
86
87 >>> def find_last_comment(contents):
88
89=== modified file 'lib/lp/answers/stories/this-is-a-faq.txt'
90--- lib/lp/answers/stories/this-is-a-faq.txt 2009-09-23 14:40:53 +0000
91+++ lib/lp/answers/stories/this-is-a-faq.txt 2009-09-24 17:29:11 +0000
92@@ -104,7 +104,7 @@
93
94 >>> def print_question_status(browser):
95 ... print extract_text(
96- ... find_tag_by_id(browser.contents, 'question-details'))
97+ ... find_tag_by_id(browser.contents, 'registration'))
98
99 >>> print_question_status(user_browser)
100 Answered ...
101
102=== modified file 'lib/lp/answers/templates/question-index.pt'
103--- lib/lp/answers/templates/question-index.pt 2009-09-03 20:22:39 +0000
104+++ lib/lp/answers/templates/question-index.pt 2009-09-24 17:29:11 +0000
105@@ -18,12 +18,7 @@
106 </style>
107 </head>
108 <body>
109- <metal:heading fill-slot="heading">
110- <h1 tal:content="context/title">Bar doesn't work</h1>
111- <div id="question-details">
112- <img alt="Open" title="Open" src="/@@/question"
113- tal:attributes="alt context/status/title;
114- title context/status/title;" />
115+ <metal:registering fill-slot="registering">
116 <span class="questionstatusOPEN"
117 tal:attributes="class string:questionstatus${context/status/name}"
118 tal:content="context/status/title">Open</span>
119@@ -48,14 +43,8 @@
120 <a
121 tal:attributes="href context/owner/fmt:url"
122 tal:content="context/owner/displayname">Foo Bar</a>
123- </div>
124-
125- <div id="question-lang">
126- <b>Language:</b>
127- <span tal:replace="context/language/englishname">English</span><br />
128- </div>
129-
130- </metal:heading>
131+ </metal:registering>
132+
133
134 <metal:portlets fill-slot="side">
135 <div tal:replace="structure context/@@+global-actions" />
136@@ -67,6 +56,11 @@
137
138 <div metal:fill-slot="main">
139
140+ <div id="question-lang" style="margin-bottom: 1em">
141+ <b>Language:</b>
142+ <span tal:replace="context/language/englishname">English</span><br />
143+ </div>
144+
145 <tal:description
146 define="global description context/description/fmt:obfuscate-email/fmt:text-to-html" />
147
148
149=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-create-question.txt'
150--- lib/lp/bugs/stories/bugs/xx-bug-create-question.txt 2009-09-23 14:40:53 +0000
151+++ lib/lp/bugs/stories/bugs/xx-bug-create-question.txt 2009-09-24 17:29:11 +0000
152@@ -241,8 +241,8 @@
153 >>> print user_browser.title
154 Questions : Jokosher
155
156- >>> content = find_main_content(user_browser.contents)
157- >>> print extract_text(content(['img'], src='/@@/question')[0].parent)
158+ >>> print extract_text(
159+ ... find_tag_by_id(user_browser.contents, 'registration'))
160 Open Question #..., asked ... by Foo Bar
161
162 No Privileges Person uses his browser's back button to view the bug

Subscribers

People subscribed via source and target branches

to status/vote changes: