Gavin Panella wrote:
> Review: Needs Fixing code
> Hi Michael,
> 
> Nice feature :) A few tiny comments, but also a possible security hole
> later on, so Needs Fixing.

Hi Gavin, thanks for the review.  I think I've addressed your comments
-- diff attached -- so please have another look.

>> === modified file 'lib/lp/code/browser/codeimport.py'
>> --- lib/lp/code/browser/codeimport.py	2010-01-20 03:19:44 +0000
>> +++ lib/lp/code/browser/codeimport.py	2010-03-05 13:53:19 +0000
>> @@ -67,7 +67,7 @@
>>      text = u'Code Import System'
>>
>>
>> -class ReviewStatusDropdownWidget(LaunchpadDropdownWidget):
>> +class DropdownWidgetWithAny(LaunchpadDropdownWidget):
>>      """A <select> widget with a more appropriate 'no value' message.
>>
>>      By default `LaunchpadDropdownWidget` displays 'no value' when the
>> @@ -88,9 +88,15 @@
>>          status_field = Choice(
>>              __name__='status', title=_("Review Status"),
>>              vocabulary=CodeImportReviewStatus, required=False)
>> -        self.status_widget = CustomWidgetFactory(ReviewStatusDropdownWidget)
>> +        self.status_widget = CustomWidgetFactory(DropdownWidgetWithAny)
>>          setUpWidget(self, 'status',  status_field, IInputWidget)
>>
>> +        type_field = Choice(
> 
> This is referred to as rcs_type later in initialize() and as an arg to
> self.context.search(), so perhaps rename this to rcs_type_field?
> 
>> +            __name__='type', title=_("Review Status"),
> 
> s/Review Status/Revision Control System/ ?

Actually, I think it's much clearer to use copy_field instead of what I
was doing there for both fields, so I did that, and renamed everything
to be hopefully more consistent.

>> +            vocabulary=RevisionControlSystems, required=False)
>> +        self.type_widget = CustomWidgetFactory(DropdownWidgetWithAny)
>> +        setUpWidget(self, 'type',  type_field, IInputWidget)
>> +
>>          # status should be None if either (a) there were no query arguments
>>          # supplied, i.e. the user browsed directly to this page (this is when
>>          # hasValidInput returns False) or (b) the user chose 'Any' in the
>> @@ -99,11 +105,12 @@
>>          status = None
>>          if self.status_widget.hasValidInput():
>>              status = self.status_widget.getInputValue()
>> +        # Similar for 'type'
>> +        rcs_type = None
>> +        if self.type_widget.hasValidInput():
>> +            rcs_type = self.type_widget.getInputValue()
>>
>> -        if status is not None:
>> -            imports = self.context.search(review_status=status)
>> -        else:
>> -            imports = self.context.getAll()
>> +        imports = self.context.search(review_status=status, rcs_type=rcs_type)
>>
>>          self.batchnav = BatchNavigator(imports, self.request)
>>

>> === modified file 'lib/lp/code/stories/codeimport/xx-codeimport-view.txt'
>> --- lib/lp/code/stories/codeimport/xx-codeimport-view.txt	2009-12-07 08:57:49 +0000
>> +++ lib/lp/code/stories/codeimport/xx-codeimport-view.txt	2010-03-05 13:53:19 +0000
>> @@ -35,9 +35,9 @@
>>  Filtering the code import list
>>  ==============================
>>
>> -The code import listing is filterable, though only on review status so
>> -far.  There are no invalid imports in the sample data, so if we filter
>> -just on them we'll see the "no imports found" message.  It is worth
>> +The code import listing is filterable, on review status and type.
>> +There are no invalid imports in the sample data, so if we filter just
>> +on them we'll see the "no imports found" message.  It is worth
>>  ensuring that the control for filtering on review status reads "Any"
>>  by default, as the code that ensures this is poking at Zope 3
>>  internals a bit.
>> @@ -64,6 +64,16 @@
>>      gnome-terminal/import
>>      evolution/import
>>
>> +We can also filter by type.
>> +
>> +    >>> browser.getControl(name="field.type").displayValue = ["Concurrent Versions System"]
> 
> This line is a bit long.

Wrapped.

>> +    >>> browser.getControl(name="submit").click()
>> +    >>> table = find_tag_by_id(browser.contents, 'code-import-listing')
>> +    >>> names = [extract_text(tr.td) for tr in table.tbody('tr')]
>> +    >>> for name in names:
>> +    ...     print name
>> +    evolution/import
>> +
>>  If we create a lot of imports, the listing view will be batched.
>>
>>      >>> from canonical.launchpad.ftests import login, logout
>>
>> === modified file 'lib/lp/code/templates/codeimport-list.pt'
>> --- lib/lp/code/templates/codeimport-list.pt	2009-08-18 00:19:52 +0000
>> +++ lib/lp/code/templates/codeimport-list.pt	2010-03-05 13:53:19 +0000
>> @@ -19,6 +19,12 @@
>>        <option label="NEW">New</option>
>>        <option label="REVIEWED">Reviewed</option>
>>      </select>
>> +    <select name="rcs_type" tal:replace="structure view/type_widget">
>> +      <option label="">Any</option>
>> +      <option label="GIT">Git</option>
>> +      <option label="BZR_SVN">Subversion</option>
>> +      <option label="SVN">Subversion (legacy)</option>
>> +    </select>
>>      <input type="submit" name="submit"/>
>>    </form>
>>
>> @@ -38,6 +44,12 @@
>>              Created
>>            </th>
>>            <th>
>> +            Type
>> +          </th>
>> +          <th>
>> +            Location
>> +          </th>
>> +          <th>
>>              Status
>>            </th>
>>          </tr>
>> @@ -60,6 +72,14 @@
>>              some date
>>            </td>
>>
>> +          <td tal:content="structure codeimport/rcs_type/title">
> 
> Is that meant to be a structure? That smells like an opportunity for
> script injection.

Hm, not really a vulnerability as we completely control the rcs type
enum, but also pointless.  structure removed.

>> +            some type
>> +          </td>
>> +
>> +          <td tal:content="structure codeimport/getImportDetailsForDisplay">
> 
> Again, perhaps not a structure?

Ditto.

> Also, are there any tests for this?

I beefed up the tests in the page test a bit -- see what you think.

>> +            some details
>> +          </td>
>> +
>>            <td tal:content="codeimport/review_status/title">
>>              status
>>            </td>
>>
> 

Thanks again for the review.

Cheers,
mwh