Merge lp:~kissiel/checkbox/fix-1492320-comment-on-manual-fail into lp:checkbox

[Maciej Kisielewski]

This MR makes checkboxes ask for comment when the user manually fails the test.

7dc29b7 plainbox:unit:job: add validation of 'explicit-fail' flag
9495c9b plainbox:unit:job: fix pep-8 issues
9e335d0 checkbox-touch: support 'explicit-fail' flag
b2c842d plainbox:commands:run: support 'explicit-fail' in plainbox and checkbox-ng
b2d9c14 plainbox:docs: mention 'explicit-fail' in the changelog
31db2cc plainbox:docs:manpages: mention 'explicit-fail' in job unit manpage

[lp:~kissiel/checkbox/fix-1492320-comment-on-manual-fail]

4092. By Maciej Kisielewski on 2015-11-04

plainbox:unit:job: add validation of 'explicit-fail' flag

This patch adds validation of 'explicit-fail' flag.

Signed-off-by: Maciej Kisielewski <email address hidden>

4093. By Maciej Kisielewski on 2015-11-04

plainbox:unit:job: fix pep-8 issues

Signed-off-by: Maciej Kisielewski <email address hidden>

4094. By Maciej Kisielewski on 2015-11-04

checkbox-touch: support 'explicit-fail' flag

This patch makes checkbox-touch display the 'add-comment' popup when user fails
a job that flagged as 'explicit-fail'.

Fixes: https://bugs.launchpad.net/checkbox-converged/+bug/1492320

Signed-off-by: Maciej Kisielewski <email address hidden>

4095. By Maciej Kisielewski on 2015-11-04

plainbox:commands:run: support 'explicit-fail' in plainbox and checkbox-ng

This patch brings support for 'explicit-fail' flag to CLI flavours of checkbox.

Fixes: https://bugs.launchpad.net/checkbox-converged/+bug/1492320

Signed-off-by: Maciej Kisielewski <email address hidden>

4096. By Maciej Kisielewski on 2015-11-04

plainbox:docs: mention 'explicit-fail' in the changelog

Signed-off-by: Maciej Kisielewski <email address hidden>

4097. By Maciej Kisielewski on 2015-11-04

plainbox:docs:manpages: mention 'explicit-fail' in job unit manpage

Signed-off-by: Maciej Kisielewski <email address hidden>

[Paul Larson (pwlars)]

I don't know that I have an easy way to test this personally, but happy to try if you can walk me through it. Otherwise, I can give it a +1 as a sanity check.

review: Approve

[Zygmunt Krynicki (zyga)]

Looks good.

I'd like to stress that we need to figure out a way to share improvements like that with session assistant-based applications. We should plan a sprint around the user interface re-usability class so that each new feature transparently lands in all applications based on SA (unless they opt-out by subclassing something)

review: Approve

=== modified file 'checkbox-touch/checkbox-touch.qml'
--- checkbox-touch/checkbox-touch.qml	2015-11-02 09:09:00 +0000
+++ checkbox-touch/checkbox-touch.qml	2015-11-04 14:36:11 +0000
@@ -667,7 +667,20 @@
 
     function showVerificationScreen(test) {
         var verificationPage = createPage("components/TestVerificationPage.qml", test);
-        verificationPage.testDone.connect(completeTest);
+        var maybeCommentVerification = function(test) {
+            if (test.outcome == 'fail' &&
+                test.flags.indexOf('explicit-fail') > -1) {
+                commentsDialog.commentDefaultText = test["comments"] || "";
+                commentsDialog.commentAdded.connect(function(comment) {
+                    test["comments"] = comment;
+                    completeTest(test);
+                });
+                PopupUtils.open(commentsDialog.dialogComponent);
+            } else {
+                completeTest(test);
+            }
+        }
+        verificationPage.testDone.connect(maybeCommentVerification);
         pageStack.push(verificationPage);
     }
     function getSubmissionInput(continuation, cancelContinuation) {

=== modified file 'plainbox/docs/changelog.rst'
--- plainbox/docs/changelog.rst	2015-11-04 08:36:33 +0000
+++ plainbox/docs/changelog.rst	2015-11-04 14:36:11 +0000
@@ -16,6 +16,11 @@
   option when you know your way around, and you want to quickly start
   developing plainbox jobs without any other jobs polluting your provider.
 
+* Plainbox now supports a new flag :ref:`explicit-fail
+  <job_flag_explicit_fail>`. Using that flag makes manual failing of the job
+  require a comment to be entered. This flag naturally makes sense only for
+  'manual', 'user-interact-verify', 'user-verify' jobs.
+
 .. _version_0_24:
 
 Plainbox 0.24

=== modified file 'plainbox/docs/manpages/plainbox-job-units.rst'
--- plainbox/docs/manpages/plainbox-job-units.rst	2015-09-28 13:20:53 +0000
+++ plainbox/docs/manpages/plainbox-job-units.rst	2015-11-04 14:36:11 +0000
@@ -265,6 +265,12 @@
         Attach this flag to jobs that cause killing of plainbox process during
         their operation. E.g. run shutdown, reboot, etc.
 
+.. _job_flag_explicit_fail:
+
+    ``explicit-fail``:
+        Use this flag to make entering comment mandatory, when the user
+        manually fails the job.
+
 .. _job_flag_has_leftovers:
 
     ``has-leftovers``:

=== modified file 'plainbox/plainbox/impl/commands/inv_run.py'
--- plainbox/plainbox/impl/commands/inv_run.py	2015-09-25 07:36:02 +0000
+++ plainbox/plainbox/impl/commands/inv_run.py	2015-11-04 14:36:11 +0000
@@ -929,9 +929,17 @@
                 print("  " + _("comments") + ": {0}".format(
                     self.C.CYAN(result.comments, bright=False)))
             cmd = self._pick_action_cmd(allowed_actions)
+            # let's store new_comment early for verification if comment has
+            # already been added in the current UI step
+            new_comment = ''
             if cmd == 'set-pass':
                 result_builder.outcome = IJobResult.OUTCOME_PASS
             elif cmd == 'set-fail':
+                if 'explicit-fail' in job.get_flag_set() and not new_comment:
+                    new_comment = input(self.C.BLUE(
+                        _('Please enter your comments:') + '\n'))
+                    if new_comment:
+                        result_builder.add_comment(new_comment)
                 result_builder.outcome = IJobResult.OUTCOME_FAIL
             elif cmd == 'set-skip' or cmd is None:
                 result_builder.outcome = IJobResult.OUTCOME_SKIP

=== modified file 'plainbox/plainbox/impl/unit/job.py'
--- plainbox/plainbox/impl/unit/job.py	2015-09-28 13:20:53 +0000
+++ plainbox/plainbox/impl/unit/job.py	2015-11-04 14:36:11 +0000
@@ -455,8 +455,8 @@
             for stage in ['purpose', 'steps', 'verification']:
                 stage_value = self.get_translated_record_value(stage)
                 if stage_value is not None:
-                    tr_description += (tr_stages[stage] + ':\n'
-                                       + stage_value + '\n')
+                    tr_description += (tr_stages[stage] + ':\n' +
+                                       stage_value + '\n')
             tr_description = tr_description.strip()
             if not tr_description:
                 # combining new description yielded empty string
@@ -800,8 +800,8 @@
                              " fields. See http://plainbox.readthedocs.org"
                              "/en/latest/author/faq.html#faq-2"),
                     onlyif=lambda unit:
-                    unit.startup_user_interaction_required
-                    and unit.get_record_value('summary') is None),
+                    unit.startup_user_interaction_required and
+                    unit.get_record_value('summary') is None),
             ],
             fields.steps: [
                 TranslatableFieldValidator,
@@ -817,9 +817,9 @@
                 TranslatableFieldValidator,
                 PresentFieldValidator(
                     severity=Severity.advice,
-                    message = ("please use purpose, steps, and verification"
-                               " fields. See http://plainbox.readthedocs.org"
-                               "/en/latest/author/faq.html#faq-2"),
+                    message=("please use purpose, steps, and verification"
+                             " fields. See http://plainbox.readthedocs.org"
+                             "/en/latest/author/faq.html#faq-2"),
                     onlyif=lambda unit: unit.plugin in (
                         'manual', 'user-verify', 'user-interact-verify')),
             ],
@@ -957,6 +957,16 @@
                         ' non-C locale then set the preserve-locale flag'
                     ),
                     onlyif=lambda unit: unit.command),
+                CorrectFieldValueValidator(
+                    lambda value, unit: (
+                        not ('explicit-fail' in unit.get_flag_set() and
+                             unit.plugin in {
+                                 'shell', 'user-interact', 'attachment',
+                                 'local', 'resource'})),
+                    Problem.useless, Severity.advice,
+                    message=_('explicit-fail makes no sense for job which '
+                              'outcome is automatically determined.')
+                ),
                 # The has-leftovers flag is useless without a command
                 CorrectFieldValueValidator(
                     lambda value, unit: (
@@ -976,13 +986,13 @@
                     lambda value: value.endswith('.qml'),
                     Problem.wrong, Severity.advice,
                     message=_('use the .qml extension for all QML files'),
-                    onlyif=lambda unit: (unit.plugin == 'qml'
-                                         and unit.qml_file)),
+                    onlyif=lambda unit: (unit.plugin == 'qml' and
+                                         unit.qml_file)),
                 CorrectFieldValueValidator(
                     lambda value, unit: os.path.isfile(unit.qml_file),
                     message=_('please point to an existing QML file'),
-                    onlyif=lambda unit: (unit.plugin == 'qml'
-                                         and unit.qml_file)),
+                    onlyif=lambda unit: (unit.plugin == 'qml' and
+                                         unit.qml_file)),
             ],
             fields.certification_status: [
                 UntranslatableFieldValidator,

=== modified file 'plainbox/plainbox/impl/unit/test_job.py'
--- plainbox/plainbox/impl/unit/test_job.py	2015-09-25 08:39:43 +0000
+++ plainbox/plainbox/impl/unit/test_job.py	2015-11-04 14:36:11 +0000
@@ -736,6 +736,17 @@
             issue_list, self.unit_cls.Meta.fields.flags,
             Problem.expected_i18n, Severity.advice, message)
 
+    def test_flags__usless_explicit_fail_on_shell_jobs(self):
+        message = ("field 'flags', explicit-fail makes no sense for job which "
+                   "outcome is automatically determined.")
+        issue_list = self.unit_cls({
+            'plugin': 'shell',
+            'flags': 'explicit-fail'
+        }, provider=self.provider).check()
+        self.assertIssueFound(
+            issue_list, self.unit_cls.Meta.fields.flags,
+            Problem.useless, Severity.advice, message)
+
 
 class JobDefinitionValidatorTests(TestCase):