Merge lp:~kissiel/checkbox/fix-1501354-back-after-testplan-selected into lp:checkbox

[Maciej Kisielewski]

This MR fixes a problem, where navigating back to select different test plan would break the app.

aa66fbf plainbox:session:assistant: be more liberal about calling get_session_{dir,id}
112da1b checkbox-touch: let changing of test plans
9ba76c1 checkbox-touch: update sessionDir after selecting new test plan
3f6b13b plainbox:session:assistant: fix PEP8 issues
2ed4471 plainbox:session:assistant: remove old, dead code

[Zygmunt Krynicki (zyga)]

+1

For some context:

09:58 <@zyga> kissiel: https://code.launchpad.net/~zyga/checkbox/sa-api-changes/+merge/273502
09:58 <@zyga> kissiel: have a look and tell me what you think
09:58 < kissiel> ack
09:58 <@zyga> kissiel: I'll propagate this down to all the apps if you ack
10:00 < kissiel> zyga: those pep8 got fixed on the branch I proposed yesterday
10:01 <@zyga> kissiel: heh
10:01 <@zyga> kissiel: where? let's merge em
10:01 < kissiel> zyga: https://code.launchpad.net/~kissiel/checkbox/fix-1501354-back-after-testplan-selected/+merge/273460
10:02 * zyga reads
10:02 <@zyga> kissiel: can you ask the sa if a test plan was set?
10:02 <@zyga> kissiel: I'd like to minimize the state that apps need to have
10:03 < kissiel> zyga: I tried that approach
10:03 < kissiel> zyga: and do all the logic in SA
10:03 <@zyga> kissiel: what is the timestamp for?
10:03 < kissiel> zyga: there are problems with resuming session
10:03 <@zyga> kissiel: I remember that
10:04 <@zyga> kissiel: what I mean specifically is why don't we expose sa._manager.test_plans (read only)
10:04 < kissiel> zyga: timestamp goes to the app-blob
10:04 <@zyga> kissiel: so that app doesn't need to keep this in this case
10:05 < kissiel> zyga: the thing is you can have boostrap already done
10:05 <@zyga> kissiel: we already track test plan selection
10:05 < kissiel> zyga: and we want to get rid of all the progress
10:05 <@zyga> kissiel: how does bootstrap prevents this?
10:05 < kissiel> zyga: and I wouldn't like to expose job-state-map
10:05 <@zyga> kissiel: I understand, you want a new session
10:05 <@zyga> kissiel: all I'm saying that you store something wa already store
10:06 < kissiel> zyga: ok, so once again, app stores it, because when resuming session app will have it set to none
10:06 <@zyga> kissiel: c-c stores test plan that sa already keeps track of there, that if could have been: if sa.selected_test_plan: ...
10:06 < kissiel> zyga: app differentiates between resuming and starting
10:06 < kissiel> sa does not
10:06 <@zyga> kissiel: are you sure?
10:06 < kissiel> zyga: spent quite a bit of time on that yesterday
10:06 <@zyga> kissiel: it pretty much seeems that sa._manager.test_plans is exactly that
10:07 <@zyga> kissiel: what am I missing then?
10:07 < kissiel> zyga: when the session is resumed, sa will start a session out of a checkpoint, right?
10:08 < kissiel> zyga: and when we change the testplan (by navigating back and forth) bootstrapping is done for each tp selected
10:08 < kissiel> and once you go back, you need to restat
10:08 < kissiel> when resuming, we couldn't see if we need to drop the progress already done or not
10:08 < kissiel> becase we 'just already have a test plan selected in sa'
10:11 <@zyga> kissiel: I need to look into this
10:11 <@zyga> kissiel: I think we should not need to re-boostrap the whole way in this case, it's just unfortunate effect of how the gui design and API doesn't let you do what you want
10:12 <@zyga> kissiel: I think that what we have now is coarse-grained for the problem (we nuke and restart which is expensive)
10:12 < kissiel> zyga: yeah, it would be nice if session could stay, and tp could change
10:13 <@zyga> kissiel: I think we ...

review: Approve

=== modified file 'checkbox-touch/components/CheckboxTouchApplication.qml'
--- checkbox-touch/components/CheckboxTouchApplication.qml	2015-09-04 13:14:04 +0000
+++ checkbox-touch/components/CheckboxTouchApplication.qml	2015-10-05 19:12:34 +0000
@@ -95,7 +95,11 @@
         });
     }
     function rememberTestplan(testplan, continuation) {
-        request("remember_testplan", [testplan], continuation, function(error) {
+        var handleResult = function(result) {
+            sessionDir = result['session_dir'];
+            continuation();
+        }
+        request("remember_testplan", [testplan], handleResult, function(error) {
             console.error("Unable to save testplan selection");
         });
     }

=== modified file 'checkbox-touch/py/checkbox_touch.py'
--- checkbox-touch/py/checkbox_touch.py	2015-09-11 12:52:09 +0000
+++ checkbox-touch/py/checkbox_touch.py	2015-10-05 19:12:34 +0000
@@ -145,6 +145,7 @@
         self._password = None
         self._timestamp = None
         self._latest_session = None
+        self.test_plan_id = None
         self.resume_candidate_storage = None
         self.assistant.use_alternate_repository(
             self._get_app_cache_directory())
@@ -238,9 +239,21 @@
     @view
     def remember_testplan(self, test_plan_id):
         """Pick the test plan as the one in force."""
+        if self.test_plan_id:
+            # test plan has been previously selected. User changed mind, we
+            # have to abandon the session
+            self.assistant.finalize_session()
+            self.assistant.start_new_session('Checkbox Converged session')
+            self._timestamp = datetime.datetime.utcnow().isoformat()
         self.test_plan_id = test_plan_id
         self.assistant.select_test_plan(test_plan_id)
         self.assistant.bootstrap()
+        # because session id (and storage) might have changed, let's share this
+        # info with the qml side
+        return {
+            'session_id': self.assistant.get_session_id(),
+            'session_dir': self.assistant.get_session_dir()
+        }
 
     @view
     def get_categories(self):

=== modified file 'plainbox/plainbox/impl/session/assistant.py'
--- plainbox/plainbox/impl/session/assistant.py	2015-09-11 12:49:39 +0000
+++ plainbox/plainbox/impl/session/assistant.py	2015-10-05 19:12:34 +0000
@@ -589,13 +589,6 @@
         UsageExpectation.of(self).enforce()
         test_plan = self._context.get_unit(test_plan_id, 'test plan')
         self._manager.test_plans = (test_plan, )
-        if False:
-            """
-            desired_job_list = select_jobs(
-                self._context.state.job_list, [unit.get_qualifier()])
-            self._context.state.update_desired_job_list(desired_job_list)
-            self._metadata.flags = {'incomplete'}
-            """
         self._manager.checkpoint()
         UsageExpectation.of(self).allowed_calls = {
             self.bootstrap: "to run the bootstrap process"
@@ -731,7 +724,6 @@
             [plan.get_qualifier() for plan in self._manager.test_plans])
         self._context.state.update_desired_job_list(desired_job_list)
 
-
     @raises(KeyError, UnexpectedMethodCall)
     def get_job_state(self, job_id: str) -> 'JobState':
         """
@@ -1240,8 +1232,8 @@
             self.get_category: "to access the definition of ant category",
             self.get_participating_categories: (
                 "to access participating categories"),
-            self.filter_jobs_by_categories: ("to select the jobs that match"
-                "particular category"),
+            self.filter_jobs_by_categories: (
+                "to select the jobs that match particular category"),
             self.remove_all_filters: "to remove all filters",
             self.get_static_todo_list: "to see what is meant to be executed",
             self.get_dynamic_todo_list: "to see what is yet to be executed",
@@ -1253,6 +1245,9 @@
             self.export_to_transport: "to export the results and send them",
             self.export_to_file: "to export the results to a file",
             self.finalize_session: "to mark the session as complete",
+            self.get_session_id: "to get the id of currently running session",
+            self.get_session_dir: ("to get the path where current session is"
+                                   "stored"),
         }