Merge lp:~sylvain-pineau/checkbox/gui_filter_testplan_by_id into lp:checkbox

[Sylvain Pineau]

This MR allow gui launchers to work correctly with testplans since the whitelist_filter settings have to matching patterns again testplan id instead of testplan names.

Only checkbox-gui is patched, the existing launchers will continue to work.

[Zygmunt Krynicki (zyga)]

14:56 <@zyga> spineau: revno 3593 does two logical changes, could you
split it please 14:56 <@zyga> spineau: specifically it doesn't say why
you use partial_id instead of the test plan name 14:57 <@zyga>
spineau: I think that using partial_id is asking for dbus path is
asking for trouble, could you use a hash of the full id insteda?
partial_id *will* clash and we'll spend a lot of time trying to debug
weird bugs from CE-QA 14:58 <@zyga> spineau: as for the second patch,
why are you ding the isValid() check there?

On Thu, Feb 26, 2015 at 2:20 PM, Sylvain Pineau
<email address hidden> wrote:
> Sylvain Pineau has proposed merging lp:~sylvain-pineau/checkbox/gui_filter_testplan_by_id into lp:checkbox.
>
> Requested reviews:
> Checkbox Developers (checkbox-dev)
>
> For more details, see:
> https://code.launchpad.net/~sylvain-pineau/checkbox/gui_filter_testplan_by_id/+merge/251080
>
> This MR allow gui launchers to work correctly with testplans since the whitelist_filter settings have to matching patterns again testplan id instead of testplan names.
>
> Only checkbox-gui is patched, the existing launchers will continue to work.
> --
> You are subscribed to branch lp:checkbox.
>
> === modified file 'checkbox-gui/checkbox-gui/WhiteListModelFactory.cpp'
> --- checkbox-gui/checkbox-gui/WhiteListModelFactory.cpp 2014-03-18 16:21:43 +0000
> +++ checkbox-gui/checkbox-gui/WhiteListModelFactory.cpp 2015-02-26 13:19:17 +0000
> @@ -19,6 +19,8 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#include <QtDBus/QtDBus>
> +#include "../gui-engine/PBNames.h"
> #include "WhiteListModelFactory.h"
>
> ListModel* WhiteListModelFactory::CreateWhiteListModel(ListModel *model, const QString &filter)
> @@ -58,10 +60,22 @@
> while(iter != paths_and_names.end() ) {
> if (rx.exactMatch(iter.value())) {
> qDebug() << iter.key().path();
> - qDebug() << " Name: " << iter.value();
> - model->appendRow(new WhiteListItem(iter.value(), \
> - iter.key().path(), \
> - model));
> + // Connect to the introspectable interface
> + QDBusInterface introspect_iface(PBBusName, \
> + iter.key().path(), \
> + "org.freedesktop.DBus.Properties", \
> + QDBusConnection::sessionBus());
> + if (introspect_iface.isValid()) {
> + QDBusReply<QVariant> reply = introspect_iface.call("Get", \
> + "com.canonical.certification.PlainBox.WhiteList1", \
> + "name");
> + QVariant var(reply);
> + QString name(var.toString());
> + qDebug() << " Name: " << name;
> + model->appendRow(new WhiteListItem(name, \
> + iter.key().path(), \
> + model));
> + }
> }
> iter++;
> }
>
> === modified file 'checkbox-gui/gui-engine/gui-engine.cpp'
> --- ch...

[Sylvain Pineau (sylvain-pineau)]

cleaned commit history + unit checksum for testplan dbus object path

review: Needs Resubmitting

[lp:~sylvain-pineau/checkbox/gui_filter_testplan_by_id]

3594. By Sylvain Pineau on 2015-02-26

checkbox-ng:service: Use the unit.checksum for testplan object path

Instead of testplan names which may conflicts between providers, the dbus
object path for testplan is the unit checksum.

3595. By Sylvain Pineau on 2015-02-26

checkbox-ng:service: Expose the testplan partial_id property over dbus

Testplan partial_id will be used by launchers whitelist_filter regexp.
The test plan names will only be used for display.

3596. By Sylvain Pineau on 2015-02-26

checkbox-gui:gui-engine: paths_and_names values are now testplan partial_id

the gui launcher whitelist_filter was designed to find a match on id, not names.

3597. By Sylvain Pineau on 2015-02-26

checkbox-gui:WhiteListModelFactory: Use the testplan partial_id for rx.exactMatch()

Now that testplans are exposed over dbus, we can't use the testplan names
to find units matching the whitelist_filter of gui lauchers.

3598. By Sylvain Pineau on 2015-02-26

checkbox-gui:WhiteListModelFactory: Use testplan names for display

gui-engine is now getting the partial_id property to populate the paths_and_names
dictionary but WhiteListModelFactory::CreateWhiteListModel() will instead use
the testplan name for the model.

[Zygmunt Krynicki (zyga)]

Thanks, +1

review: Approve

=== modified file 'checkbox-gui/checkbox-gui/WhiteListModelFactory.cpp'
--- checkbox-gui/checkbox-gui/WhiteListModelFactory.cpp	2014-03-18 16:21:43 +0000
+++ checkbox-gui/checkbox-gui/WhiteListModelFactory.cpp	2015-02-26 15:17:26 +0000
@@ -58,10 +58,22 @@
     while(iter != paths_and_names.end() ) {
         if (rx.exactMatch(iter.value())) {
             qDebug() << iter.key().path();
-            qDebug() << " Name: " << iter.value();
-            model->appendRow(new WhiteListItem(iter.value(), \
-                                               iter.key().path(), \
-                                               model));
+            // Connect to the introspectable interface
+            QDBusInterface introspect_iface(PBBusName, \
+                                 iter.key().path(), \
+                                 "org.freedesktop.DBus.Properties", \
+                                 QDBusConnection::sessionBus());
+            if (introspect_iface.isValid()) {
+                QDBusReply<QVariant> reply  = introspect_iface.call("Get", \
+                           "com.canonical.certification.PlainBox.WhiteList1", \
+                           "name");
+                QVariant var(reply);
+                QString name(var.toString());
+                qDebug() << " Name: " << name;
+                model->appendRow(new WhiteListItem(name, \
+                                                   iter.key().path(), \
+                                                   model));
+            }
         }
         iter++;
     }

=== modified file 'checkbox-gui/gui-engine/gui-engine.cpp'
--- checkbox-gui/gui-engine/gui-engine.cpp	2015-02-25 23:54:36 +0000
+++ checkbox-gui/gui-engine/gui-engine.cpp	2015-02-26 15:17:26 +0000
@@ -358,12 +358,12 @@
         if (introspect_iface.isValid()) {
             QDBusReply<QVariant> reply  = introspect_iface.call("Get", \
                        "com.canonical.certification.PlainBox.WhiteList1", \
-                       "name");
+                       "partial_id");
             QVariant var(reply);
-            QString name(var.toString());
-            qDebug() << name;
+            QString partial_id(var.toString());
+            qDebug() << partial_id;
             // Only show the user whitelists with the desired prefix.
-            paths_and_names.insert(child->object_path,name);
+            paths_and_names.insert(child->object_path, partial_id);
             // First time round, fill in our whitelist member
             if (!initialised) {
                 whitelist.insert(child->object_path, false);

=== modified file 'checkbox-ng/checkbox_ng/service.py'
--- checkbox-ng/checkbox_ng/service.py	2015-02-25 23:54:36 +0000
+++ checkbox-ng/checkbox_ng/service.py	2015-02-26 15:17:26 +0000
@@ -420,9 +420,7 @@
     # Some internal helpers
 
     def _get_preferred_object_path(self):
-        # TODO: this clashes with providers, maybe use a random ID instead
-        return "/plainbox/whitelist/{}".format(
-            mangle_object_path(self.native.name))
+        return "/plainbox/whitelist/{}".format(self.native.checksum)
 
     # Value added
 
@@ -433,6 +431,13 @@
         """
         return self.native.name or ""
 
+    @dbus.service.property(dbus_interface=WHITELIST_IFACE, signature="s")
+    def partial_id(self):
+        """
+        partial_id of this testplan
+        """
+        return self.native.partial_id or ""
+
 
 class JobResultWrapper(PlainBoxObjectWrapper):
     """