Merge lp:~cr3/checkbox-editor/attachments into lp:checkbox-editor

[Marc Tardif]

To test these changes, I created a whitelist using checkbox-editor without the attachments_whitelist preference disabled. I then created another whitelist with the same preference enabled and this was the diff:

2c2
< # Date: 2012-08-01 06:34:01
---
> # Date: 2012-08-01 06:34:23
21a22,42
> \_\_info\_\_
> codecs\_attachment
> cpuinfo\_attachment
> dmesg\_attachment
> dmi\_attachment
> dmidecode\_attachment
> efi\_attachment
> lspci\_attachment
> lspci\_network\_attachment
> lsusb\_attachment
> meminfo\_attachment
> modprobe\_attachment
> modules\_attachment
> sysctl\_attachment
> sysfs\_attachment
> udev\_attachment
> gcov\_attachment
> lsmod\_attachment
> acpi\_sleep\_attachment
> installer\_bootchart\.tgz
> installer\_debug\.gz

Looks good to me!

[Javier Collado (javier.collado)]

Looking at the merge proposal, it's not clear to me why it's needed to check 'depends' and 'name' fields to whitelist jobs based on the plugin type like this:

if ('depends' not in description
    and '/' not in description['name']
    and description.get('plugin') == plugin):

Please could you add a brief explanation as a comment to the code for future reference?

review: Needs Information

[lp:~cr3/checkbox-editor/attachments]

149. By Marc Tardif on 2012-08-02

Added comments when checking for resource and attachment plugins.

[Marc Tardif (cr3)]

Agreed, I added a comment and took the opportunity to refactor that logic into a function in the model module.

review: Needs Resubmitting

[Javier Collado (javier.collado)]

Thanks, it looks good now.

I've also run a test to generate a whitelist with/without the attachments option selected and the diff between the two of them makes sense.

review: Approve

=== added file 'README'
--- README	1970-01-01 00:00:00 +0000
+++ README	2012-08-02 01:37:20 +0000
@@ -0,0 +1,21 @@
+README
+======
+
+Editor for the checkbox test integration framework.
+
+
+Building
+--------
+
+To build the project, install the packager package and run:
+
+    packager config set extension 'mako'
+    packager render
+
+
+Running
+-------
+
+To run the project, build it and run:
+
+    PYTHONPATH=. ./bin/checkbox-editor

=== modified file 'checkbox_editor/editor.py'
--- checkbox_editor/editor.py	2011-10-10 13:12:05 +0000
+++ checkbox_editor/editor.py	2012-08-02 01:37:20 +0000
@@ -879,8 +879,9 @@
             """
             Return whitelist patterns for selected row
             """
+            whitelist_attachments = self.preferences.general['whitelist_attachments']
             whitelist_resources = self.preferences.general['whitelist_resources']
-            for row in model.get_selected_rows(include_resources=whitelist_resources):
+            for row in model.get_selected_rows(include_attachments=whitelist_attachments, include_resources=whitelist_resources):
                 description = row[model.DESCRIPTION]
                 if description and 'name' in description:
                     if ('plugin' in description

=== modified file 'checkbox_editor/glade/preferences.glade'
--- checkbox_editor/glade/preferences.glade	2011-08-05 16:57:03 +0000
+++ checkbox_editor/glade/preferences.glade	2012-08-02 01:37:20 +0000
@@ -384,6 +384,27 @@
                   </packing>
                 </child>
                 <child>
+                  <object class="GtkCheckButton" id="whitelist_attachments">
+                    <property name="label" translatable="yes">Whitelist attachment jobs</property>
+                    <property name="visible">True</property>
+                    <property name="can_focus">True</property>
+                    <property name="receives_default">False</property>
+                    <property name="tooltip_text" translatable="yes">All attachments will be implicitly selected when launching a whitelist so that the submission.xml file contains all necessary data</property>
+                    <property name="use_action_appearance">False</property>
+                    <property name="draw_indicator">True</property>
+                    <child internal-child="accessible">
+                      <object class="AtkObject" id="whitelist_attachments-atkobject">
+                        <property name="AtkObject::accessible-name" translatable="yes">preferences_whitelist_attachments</property>
+                      </object>
+                    </child>
+                  </object>
+                  <packing>
+                    <property name="expand">False</property>
+                    <property name="fill">True</property>
+                    <property name="position">2</property>
+                  </packing>
+                </child>
+                <child>
                   <object class="GtkCheckButton" id="whitelist_resources">
                     <property name="label" translatable="yes">Whitelist resource jobs</property>
                     <property name="visible">True</property>
@@ -401,7 +422,7 @@
                   <packing>
                     <property name="expand">False</property>
                     <property name="fill">True</property>
-                    <property name="position">2</property>
+                    <property name="position">3</property>
                   </packing>
                 </child>
                 <child>
@@ -451,7 +472,7 @@
                   <packing>
                     <property name="expand">False</property>
                     <property name="fill">True</property>
-                    <property name="position">3</property>
+                    <property name="position">4</property>
                   </packing>
                 </child>
               </object>

=== modified file 'checkbox_editor/model.py'
--- checkbox_editor/model.py	2012-07-06 09:24:48 +0000
+++ checkbox_editor/model.py	2012-08-02 01:37:20 +0000
@@ -103,41 +103,53 @@
                     yield subjob_row
 
 
-    def get_selected_rows(self, parent=None, include_resources=False):
+    def get_selected_rows(self, parent=None, include_attachments=False, include_resources=False):
         """
         Return all selected rows
         possibly including resources depending on configuration
         """
-        if include_resources:
-            def has_child_resources(row):
+        if include_resources or include_attachments:
+            def check_plugins(description, plugins):
+                # Only check descriptions without dependencies,
+                # to skip attachments output by other jobs, and names
+                # without a slash, to skip jobs in suites.
+                return (description
+                    and 'depends' not in description
+                    and '/' not in description['name']
+                    and description.get('plugin') in plugins)
+
+            def has_child_plugins(row, plugins):
                 """
-                Return True if any child is a resource
+                Return True if any child has a matching plugin
                 """
                 for child_row in self.get_all_rows(row):
-                    description = child_row[self.DESCRIPTION]
-                    if (description
-                        and description.get('plugin') == 'resource'):
+                    if check_plugins(child_row[self.DESCRIPTION], plugins):
                         return True
                 return False
 
             def is_row_selected(row):
                 """
-                Filter selected rows
-                and resources rows according to configuration
+                Filter selected rows, resources rows
+                and attachments rows according to configuration
                 """
                 # Filter selected rows
                 if row[self.SELECTED]:
                     return True
 
-                # Filter resources rows
+                # Filter resources or attachments rows
+                plugins = []
                 if self.preferences.general['whitelist_resources']:
+                    plugins.append('resource')
+                if self.preferences.general['whitelist_attachments']:
+                    plugins.append('attachment')
+
+                if plugins:
                     description = row[self.DESCRIPTION]
                     if description:
-                        plugin = description.get('plugin')
-                        if plugin == 'resource':
+                        if check_plugins(description, plugins):
                             return True
                         else:
-                            return has_child_resources(row)
+                            return has_child_plugins(row, plugins)
 
                 return False
         else:

=== modified file 'checkbox_editor/preferences.py'
--- checkbox_editor/preferences.py	2012-04-19 11:07:42 +0000
+++ checkbox_editor/preferences.py	2012-08-02 01:37:20 +0000
@@ -54,6 +54,7 @@
                         'replace_tabs_with_spaces': True,
                         'number_of_spaces': 4,
                         'checkbox_data_cleanup': True,
+                        'whitelist_attachments': True,
                         'whitelist_resources': True,
                         }
         self.environment = {}
@@ -86,7 +87,7 @@
                     if option in general_options:
                         self.general[option] = parser.get('general', option)
                 for option in ('checkbox_data_cleanup', 'replace_tabs_with_spaces',
-                               'whitelist_resources'):
+                               'whitelist_attachments', 'whitelist_resources'):
                     if option in general_options:
                         self.general[option] = parser.getboolean('general', option)
                 if 'number_of_spaces' in general_options:
@@ -325,7 +326,8 @@
     """
     GENERAL_WIDGET_NAMES = ('checkbox_binary', 'checkbox_data_cleanup',
                             'replace_tabs_with_spaces', 'number_of_spaces',
-                            'whitelist_resources', 'extra_validation_file'
+                            'whitelist_attachments', 'whitelist_resources',
+                            'extra_validation_file'
                             )
     VC_WIDGET_NAMES = ('pull_on_open', 'commit_on_save', 'push_on_close',
                        'pull_branch', 'push_branch',

=== modified file 'checkbox_editor/whitelist.py'
--- checkbox_editor/whitelist.py	2012-07-06 14:24:29 +0000
+++ checkbox_editor/whitelist.py	2012-08-02 01:37:20 +0000
@@ -7,6 +7,7 @@
 
 import re, logging
 from datetime import datetime
+from functools import partial
 
 from .data import Reporter
 from .util import handle_exceptions, MessageDialogRunner
@@ -459,18 +460,28 @@
 
         selected_patterns = []
 
+        def is_plugin(row, plugin):
+            # Only check descriptions without dependencies,
+            # to skip attachments output by other jobs, and names
+            # without a slash, to skip jobs in suites.
+            description = row[self.main_model.DESCRIPTION]
+            return (description
+                and 'depends' not in description
+                and '/' not in description['name']
+                and description.get('plugin') == plugin)
+
         if self.main_model.preferences.general['whitelist_resources']:
-            def is_resource(row):
-                description = row[self.main_model.DESCRIPTION]
-                if description:
-                    plugin = description.get('plugin')
-                    if plugin == 'resource':
-                        return True
-                return False
-            resource_rows = self.main_model.get_matching_rows(is_resource)
+            resource_func = partial(is_plugin, plugin='resource')
+            resource_rows = self.main_model.get_matching_rows(resource_func)
             for resource_row in resource_rows:
                 self.main_model.set_selected(resource_row, True)
 
+        if self.main_model.preferences.general['whitelist_attachments']:
+            attachment_func = partial(is_plugin, plugin='attachment')
+            attachment_rows = self.main_model.get_matching_rows(attachment_func)
+            for attachment_row in attachment_rows:
+                self.main_model.set_selected(attachment_row, True)
+
         for name, _ in self.model:
             references = self.main_model.name_cache[name]
             rows = (self.main_model[reference.get_path()]

=== modified file 'templates/checkbox/data/.checkbox-editor.cfg.tmpl'
--- templates/checkbox/data/.checkbox-editor.cfg.tmpl	2011-04-11 13:49:20 +0000
+++ templates/checkbox/data/.checkbox-editor.cfg.tmpl	2012-08-02 01:37:20 +0000
@@ -13,4 +13,5 @@
 replace_tabs_with_spaces = True
 number_of_spaces = 4
 checkbox_binary = bin/checkbox-{{name}}-gtk
+whitelist_attachments = True
 whitelist_resources = True