Merge lp:~brendan-donegan/checkbox/bug868941_submission_timestamp into lp:checkbox

[Brendan Donegan]

This branch modifies the plugin which generates the submission.xml and adds a new one whose responsibility it is to prompt the user whether they want to keep an existing submission.xml or save it.

The 'check-report' callback in launchpad_report determines whether there is already a submission present - it then fires 'prompt-timestamp' in the submission_prompt plugin which gets a yes/no answer from the user. This is fed back to the 'report' callback of launchpad_report which uses it to determine whether it should copy the old report to a timestamped directory, or do nothing.

[Sylvain Pineau (sylvain-pineau)]

It's a really good addition, thanks for this very useful feature.
For me it's ok to have it in the trunk.

review: Approve

[Jeff Lane  (bladernr)]

Since we're only saving a single file here (submission.xml) any particular reason for creating timestamp directories rather than just copying the file to something like checkbox-certification-TIMESTAMP.log ??

I could see that being useful if other things were saved (maybe that's worth it just for future expansion ability) but just curious. It's a personal preference thing I guess... mine would be one dir full of logs that have timestamps, rather than one dir full of subdirs each having one log (assuming multiple runs).

[Brendan Donegan (brendan-donegan)]

As you say, personal preference I guess. Thinking about it a bit more though, adding the extra directory might make it more difficult to locate the old submissions - so yeah I think having them all at the same level is better. I'll make that change.

[Brendan Donegan (brendan-donegan)]

Made that change - would really like someone to approve this and get it out of the way.

[Brendan Donegan (brendan-donegan)]

Going to have to rework this and put the change into checkbox-certification instead.

[Ara Pulido (ara)]

Instead of putting it in checkbox-certification I would:

 * Put it in checkbox with a parameter
 * In checkbox-certification the parameter is enabled by default
 * In checkbox the parameter is disabled by default

[Brendan Donegan (brendan-donegan)]

Time for someone to look at this again. At the moment it works like:

* Need to specify the -S (or --save-submission) flag for it to work in the first place
* If an existing submission is detected then it saves the current submission under a different name

[Daniel Manrique (roadmr)]

Hi! So I was trying this code and am experiencing a few problems. Here's what I observed:

- Line 76: Global variable filename doesn't exist, so filename needs to be self.filename

- Firing "report" with a "response" parameter (line 143). The extra parameter is correctly handled by the updated report method on launchpad_report.py (line 63-64), but this will fire the report method in *all* registered plugins. So I get this on the log from one of the plugins whose "report" method can't handle the extra parameter:

2011-11-14 16:42:48,991 DEBUG Calling ./plugins/system_info.py SystemInfo.report(yes) for report with priority -10.
2011-11-14 16:42:48,991 ERROR Error running event handler ./plugins/system_info.py SystemInfo.report(yes) for event type 'report'
Traceback (most recent call last):
  File "/home/roadmr/Documents/checkboxes/submission_timestamp/checkbox/reactor.py", line 74, in fire
    results.append(handler(*args, **kwargs))
TypeError: report() takes exactly 1 argument (2 given)

I get this 5 times, for the SystemInfo, ClientInfo, SubmissionInfo, DatetimeInfo and JobsPrompt plugins.

The easy way to handle this is to make all plugins recognize the extra "response" parameter but since it's only used in launchpad_report.py, I think the best solution would be one that kept this concern away from the other plugins.

- Lastly you may want to zorch the pdb-related lines (on lines 19 and 137) when you're sure they are no longer needed.

review: Needs Fixing

[Brendan Donegan (brendan-donegan)]

1. Fixed
2. Created a 'save_report' function to encapsulate the saving of the report without the 'report' function needing to know about the 'response' parameter
3. Zorched

I'm still having trouble getting a submission made on my system. I'll have a quick check to see if it's to do with these changes or if it was something that was already there and then update the review status.

[lp:~brendan-donegan/checkbox/bug868941_submission_timestamp]

1102. By Brendan Donegan on 2011-11-15

Little corrections.

1103. By Brendan Donegan on 2011-11-15

Merged from trunk.

[Daniel Manrique (roadmr)]

Hi,

Two things may have been giving you trouble when submitting:

- Missing plugin: field for attachment.png job. This is fixed in trunk.

- I kept having trouble when trying to save existing submission.xml files. Here's my test procedure (not a testcase :)

1- Running checkbox with -S and specifying one or two automated tests
2- Rerunning with -S, and specifying one or two *different* tests (unmark the previous selected ones, choose a couple of new ones)

after this I expected the submission.xml file and the submission_xxxxxxx.xml file (from the first run) to be different, but they were the same, and always the same size.

I also looked in submission.xml for the test name I ran (memory/info for instance) and it's not there, even though I'm sure it ran (it's a manual test so I had to answer it). They're basically empty and contain no test results at all, only the data from the gathering phase.

I think this is because
a) calling self.report() in check_report and save_report looks strange and seemed to be giving trouble generating the report even without -S. launchpad_report.py. Can this be changed to self._manager.reactor.fire("report")?
b) the call to report generation is only getting invoked if the user NOT choose to save the report. Method check_report. I think the logic needs to change to generate the report outside the "if" so it gets called regardless of what the user responds.

Could you validate that things behave as I describe?

review: Needs Information

[Marc Tardif (cr3)]

Gentle nudge, this merge request needs some love...

[Daniel Manrique (roadmr)]

I tested this again and I'm getting the same results as on my last comment:

1- The submission.xml doesn't contain the test results (I'm using a minimal whitelist with only 3 tests and yet none of them show).

2- Sometimes when the show_info method gets invoked, the dialog that pops up is blank and things just block, I have to forcefully kill checkbox to return things to a working state.

So as mentioned before, the logic could use some looking at and potentially fixing.

It'd be great to include this in Checkbox before Feature Freeze, so I'd appreciate if you could have a second look at this.

review: Needs Fixing

[lp:~brendan-donegan/checkbox/bug868941_submission_timestamp]

1104. By Brendan Donegan on 2012-02-07

Merged from trunk

[Brendan Donegan (brendan-donegan)]

This one has been left to the sidelines a bit. I'm revisiting it now and hopefully can iron out the problems.

[Greg Vallande (gvallande)]

@brendand: Has there been any progress on this since 02-07-12? It appears this one needs some love.

review: Needs Information

[Brendan Donegan (brendan-donegan)]

Unfortunately not :( I have to work on this branch in my spare time, which I've not been getting a lot of recently! If it's deemed to be a priority I can look at it, but I haven't seen any suggestion that it is,

[Daniel Manrique (roadmr)]

Since this feature needs some reworking anyway, I'll reject the merge request, as it's not adequate to keep it as a placeholder if no active work is being done on it. That's what the bug report is for. Thanks anyway!

review: Disapprove

Unmerged revisions

1104. By Brendan Donegan on 2012-02-07

Merged from trunk

1103. By Brendan Donegan on 2011-11-15

Merged from trunk.

1102. By Brendan Donegan on 2011-11-15

Little corrections.

1101. By Brendan Donegan on 2011-11-14

Made timestamping make more sense.

1100. By Brendan Donegan on 2011-11-14

Merged from trunk.

1099. By Brendan Donegan on 2011-11-14

Merged from trunk.

1098. By Brendan Donegan on 2011-11-11

Added option to enable saving of submission xml file.

1097. By Brendan Donegan on 2011-11-09

Merged from trunk.

1096. By Brendan Donegan on 2011-10-17

Instead of timestamped directory just timestamp the file.

1095. By Brendan Donegan on 2011-10-15

Merge from trunk.

=== modified file 'checkbox/application.py'
--- checkbox/application.py	2010-09-23 10:11:27 +0000
+++ checkbox/application.py	2012-02-07 08:00:00 +0000
@@ -87,7 +87,10 @@
         parser.add_option("-w", "--whitelist",
                           help=_("Shorthand for --config=.*/jobs_info/whitelist."))
         parser.add_option("-W", "--whitelist-file",
-                          help=_("Shorthand for --config=.*/jobs_info/whitelist_file."))
+                          help=_("Shorthand for --config=checkbox/plugins/jobs_info/whitelist_file."))
+        parser.add_option("-S", "--save-submission",
+                          action="store_true",
+                          help=_("Shorthand for --config=checkbox/plugins/submission_prompt/save_submission."))
         return parser.parse_args(args)
 
     def create_application(self, args=sys.argv):
@@ -107,6 +110,8 @@
             if value:
                 options.config.append("=".join([key, value]))
 
+        if options.save_submission:
+            options.config.append("=".join(["checkbox/plugins/launchpad_report/save_submission", str(options.save_submission)]))
         # Set logging early
         set_logging(options.log_level, options.log)
 

=== modified file 'debian/changelog'
--- debian/changelog	2012-02-06 20:04:05 +0000
+++ debian/changelog	2012-02-07 08:00:00 +0000
@@ -12,7 +12,10 @@
   [Daniel Manrique]
   * Changed way of obtaining preferred browser to ensure we honor the user's
     preference rather than Chromium's clobbering of
-    /etc/alternatives/gnome-www-browser (LP: #925603) 
+    /etc/alternatives/gnome-www-browser (LP: #925603)
+
+  [Brendan Donegan]
+  * Implement prompt for user to save the old submission if they wish to (LP: #868941)
 
  -- Daniel Manrique <[e-mail]>  Mon, 06 Feb 2012 14:55:58 -0500
 

=== modified file 'plugins/launchpad_report.py'
--- plugins/launchpad_report.py	2011-12-07 21:25:59 +0000
+++ plugins/launchpad_report.py	2012-02-07 08:00:00 +0000
@@ -17,13 +17,15 @@
 # along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
 #
 import os
+import time
+import shutil
 
 from gettext import gettext as _
 from string import printable
 
 from checkbox.lib.safe import safe_make_directory
 
-from checkbox.properties import Path
+from checkbox.properties import Path, Bool
 from checkbox.plugin import Plugin
 from checkbox.reports.launchpad_report import LaunchpadReportManager
 
@@ -39,6 +41,9 @@
     # XSL Stylesheet
     stylesheet = Path(default="%(checkbox_share)s/report/checkbox.xsl")
 
+    # Whether to ask to save the submission XML
+    save_submission = Bool(required=False)
+
     def register(self, manager):
         super(LaunchpadReport, self).register(manager)
 
@@ -67,6 +72,8 @@
             self._manager.reactor.call_on(rt, rh)
 
         # Launchpad report should be generated last.
+        self._manager.reactor.call_on("check-report", self.check_report, 100)
+        self._manager.reactor.call_on("save-report", self.save_report, 100)
         self._manager.reactor.call_on("report", self.report, 100)
 
     def report_attachments(self, attachments):
@@ -130,6 +137,26 @@
                 "comment": test.get("data", "")}
             self._report["questions"].append(question)
 
+    def check_report(self, interface):
+        if self.save_submission and os.path.exists(self.filename):
+            self._manager.reactor.fire("prompt-timestamp", interface)
+        else:
+            # Since there is no existing submission we don't want to keep it
+            self.report()
+
+    def save_report(self, response):
+        if response == 'yes':
+            # Rename the old submission including a timestamp in the path
+            timestamp = time.strftime("%Y%m%d-%H%M%S",
+                               time.localtime(os.path.getctime(self.filename)))
+
+            basename = os.path.basename(self.filename)
+            timestamped_file = os.path.splitext(basename)[0] + '_' + timestamp + os.path.splitext(basename)[1]
+
+            shutil.move(self.filename, timestamped_file)
+        
+        self.report()
+
     def report(self):
         # Prepare the payload and attach it to the form
         stylesheet_path = os.path.join(

=== modified file 'plugins/report_prompt.py'
--- plugins/report_prompt.py	2011-09-08 14:52:33 +0000
+++ plugins/report_prompt.py	2012-02-07 08:00:00 +0000
@@ -37,7 +37,7 @@
 
         if interface.direction != PREV:
             interface.show_progress(_("Building report..."),
-                self._manager.reactor.fire, "report")
+                self._manager.reactor.fire, "check-report", interface)
 
         self._manager.reactor.cancel_call(event_id)
 

=== added file 'plugins/submission_prompt.py'
--- plugins/submission_prompt.py	1970-01-01 00:00:00 +0000
+++ plugins/submission_prompt.py	2012-02-07 08:00:00 +0000
@@ -0,0 +1,40 @@
+#
+# This file is part of Checkbox.
+#
+# Copyright 2011 Canonical Ltd.
+#
+# Checkbox is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Checkbox is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+from gettext import gettext as _
+
+from checkbox.plugin import Plugin
+from checkbox.user_interface import NEXT
+
+class SubmissionPrompt(Plugin):
+
+    def register(self, manager):
+        super(SubmissionPrompt, self).register(manager)
+
+        self._manager.reactor.call_on("prompt-timestamp", self.prompt_timestamp)
+
+    def prompt_timestamp(self, interface):
+        if interface.direction == NEXT:
+            response = interface.show_info(
+                _("An existing submission file was detected.\n"
+                  "Do you want to keep it?"),
+                ["yes", "no"], "yes")
+            self._manager.reactor.fire("save-report", response)
+
+factory = SubmissionPrompt

=== modified file 'scripts/removable_storage_test' (properties changed: +x to -x)