Code review comment for lp:~bac/launchpad/bug-162754

Revision history for this message
Māris Fogels (mars) wrote :

Hi Brad,

The code in this branch looks good. I have one question about the tests, and a few suggestions for the UI text.

For the lanchpadform-view.txt you test for presence of extra elements when the widget_class field is present. This is the positive case. Do you need to test for the absence of those same elements when the widget_field attribute is missing?

Regarding the UI text, I found that some of the long descriptions for the new options made it unclear if the flag means "I am not the maintainer of this project" or "I do not want this project to be maintained". Specifically I would reword "but you don't want to actually maintain" to be "but you do not want to be the maintainer of". This confusion appears in two of the long descriptions.

The concept and role of the "Registry Administrators" is new to the user. I think that the short and long description during project creation does a good job of introducing this concept. However, I feel that the "Assign to Registry Administrators" control does not. Both controls result in the same outcome (the admins take over), but from a user's perspective the controls are completely different, mostly because both control's primary and secondary text share no similarity. To remedy this I suggest making all of the text for the two controls the same (or very very similar): "I do not want to maintain this project".

The code looks good, so I'm marking this as "Approved", but that is conditional upon a UI review from a certified reviewer, and a look at the text by mrevell.

Maris

review: Approve (code)

« Back to merge proposal