Hi Michael,

Nice feature :) A few tiny comments, but also a possible security hole
later on, so Needs Fixing.

Gavin.


> === 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/ ?

> +            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/doc/codeimport.txt'
> --- lib/lp/code/doc/codeimport.txt	2010-01-12 03:46:21 +0000
> +++ lib/lp/code/doc/codeimport.txt	2010-03-05 13:53:19 +0000
> @@ -399,22 +399,15 @@
>  Retrieving CodeImports
>  ----------------------
>
> -You can retrieve all imports with the `getAll` method of ICodeImport.
> +You can retrive subsets of code imports with the `search` method of
> +ICodeImportSet.  Passing no arguments returns all code imports.
>
> -    >>> svn_import in code_import_set.getAll()
> +    >>> svn_import in code_import_set.search()
>      True
>
> -You can also retrive an import by id, which will be used to present the
> -object view for an import and also by branch, which will be used to
> -present the import's details on the page of the branch.
> -
> -    >>> code_import_set.get(svn_import.id).url
> -    u'svn://svn.example.com/trunk'
> -    >>> code_import_set.getByBranch(cvs_import.branch).cvs_root
> -    u':pserver:<email address hidden>:/cvsroot'
> -
> -Finally, you can search for imports by review status.  For instance,
> -there is a single sample CodeImport with the "REVIEWED" status:
> +You can filter the results by review status and by type.  For
> +instance, there is a single sample CodeImport with the "REVIEWED"
> +status:
>
>      >>> reviewed_imports = list(code_import_set.search(
>      ...     review_status=CodeImportReviewStatus.REVIEWED))
> @@ -423,6 +416,35 @@
>      >>> reviewed_imports[0].review_status.name
>      'REVIEWED'
>
> +And a single Git import.
> +
> +    >>> git_imports = list(code_import_set.search(
> +    ...     rcs_type=RevisionControlSystems.GIT))
> +    >>> git_imports
> +    [<...CodeImport...>]
> +    >>> git_imports[0].rcs_type.name
> +    'GIT'
> +
> +Passing both paramters is combined as "and".
> +
> +    >>> reviewed_git_imports = list(code_import_set.search(
> +    ...     review_status=CodeImportReviewStatus.REVIEWED,
> +    ...     rcs_type=RevisionControlSystems.GIT))
> +    >>> reviewed_git_imports
> +    [<...CodeImport...>]
> +    >>> reviewed_git_imports[0].rcs_type.name
> +    'GIT'
> +    >>> reviewed_git_imports[0].review_status.name
> +    'REVIEWED'
> +
> +You can also retrive an import by id and by branch, which will be used
> +to present the import's details on the page of the branch.
> +
> +    >>> code_import_set.get(svn_import.id).url
> +    u'svn://svn.example.com/trunk'
> +    >>> code_import_set.getByBranch(cvs_import.branch).cvs_root
> +    u':pserver:<email address hidden>:/cvsroot'
> +
>  When you ask for an id that is not present ICodeImportSet.get() raises
>  canonical.launchpad.interfaces.NotFoundError, rather than some
>  internal database exception.
>
> === modified file 'lib/lp/code/interfaces/codeimport.py'
> --- lib/lp/code/interfaces/codeimport.py	2010-01-12 02:07:38 +0000
> +++ lib/lp/code/interfaces/codeimport.py	2010-03-05 13:53:19 +0000
> @@ -191,9 +191,6 @@
>              cvs_root=None, cvs_module=None, review_status=None):
>          """Create a new CodeImport."""
>
> -    def getAll():
> -        """Return an iterable of all CodeImport objects."""
> -
>      def getActiveImports(text=None):
>          """Return an iterable of all 'active' CodeImport objects.
>
> @@ -222,9 +219,11 @@
>      def delete(id):
>          """Delete a CodeImport given its id."""
>
> -    def search(review_status):
> -        """Find the CodeImports of the given status.
> +    def search(review_status=None, rcs_type=None):
> +        """Find the CodeImports of the given status and type.
>
>          :param review_status: An entry from the `CodeImportReviewStatus`
> -                              schema.
> +            schema, or None, which signifies 'any status'.
> +        :param rcs_type: An entry from the `RevisionControlSystems`
> +            schema, or None, which signifies 'any type'.
>          """
>
> === modified file 'lib/lp/code/model/codeimport.py'
> --- lib/lp/code/model/codeimport.py	2010-01-31 23:48:38 +0000
> +++ lib/lp/code/model/codeimport.py	2010-03-05 13:53:19 +0000
> @@ -31,6 +31,7 @@
>  from canonical.database.datetimecol import UtcDateTimeCol
>  from canonical.database.enumcol import EnumCol
>  from canonical.database.sqlbase import SQLBase, quote, sqlvalues
> +from canonical.launchpad.interfaces import IStore
>  from lp.code.model.codeimportjob import CodeImportJobWorkflow
>  from lp.registry.model.productseries import ProductSeries
>  from canonical.launchpad.webapp.interfaces import NotFoundError
> @@ -265,10 +266,6 @@
>              CodeImportJob.delete(code_import.import_job.id)
>          CodeImport.delete(code_import.id)
>
> -    def getAll(self):
> -        """See `ICodeImportSet`."""
> -        return CodeImport.select()
> -
>      def getActiveImports(self, text=None):
>          """See `ICodeImportSet`."""
>          query = self.composeQueryString(text)
> @@ -334,6 +331,11 @@
>          """See `ICodeImportSet`."""
>          return CodeImport.selectOneBy(branch=branch)
>
> -    def search(self, review_status):
> +    def search(self, review_status=None, rcs_type=None):
>          """See `ICodeImportSet`."""
> -        return CodeImport.selectBy(review_status=review_status)
> +        clauses = []
> +        if review_status is not None:
> +            clauses.append(CodeImport.review_status == review_status)
> +        if rcs_type is not None:
> +            clauses.append(CodeImport.rcs_type == rcs_type)
> +        return IStore(CodeImport).find(CodeImport, *clauses)
>
> === 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.

> +    >>> 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.

> +            some type
> +          </td>
> +
> +          <td tal:content="structure codeimport/getImportDetailsForDisplay">

Again, perhaps not a structure?

Also, are there any tests for this?

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