Require user to provide comment for manually skipped or failed cert-b…

…lockers (#426)

* Require user to provide comment for manually skipped or failed cert-blockers

When issuing certificates for a device, the team reviews the different
test reports generated. Jobs that have a certification status set to
"blocker" must pass in order to issue a review.

Sometimes, it is not the case, but users do not necessarily provide a
comment that would help with the review.

Semi-automated jobs (`user-interact` and `user-interact-verify` [1]) can
be skipped before being started or after they've run.

Manual jobs can be skipped after been "run", meaning after their content
has been displayed to the user and the user has to pick an outcome.

These are handled in

- checkbox_ng.launcher.stages._run_single_job_with_ui_loop
- checkbox_ng.launcher.stages._interaction_callback

Checkbox local and Checkbox remote call _interaction_callback()
differently; Checkbox remote does not provide the current job's state,
which is required to know the job's effective certification status (this
status can be set in the job definition, but overriden in the test
plan). checkbox_ng.launcher.stages._interaction_callback() signature is
modified to pass the job state.

Fix #387

[1] https://checkbox.readthedocs.io/en/latest/units/job.html#job-fields

* Ask user for a comment if trying to skip a user-interact[-verify] job on remote

Previous commit only took Checkbox local into account for
`user-interact` and `user-interact-verify` jobs through the
_run_single_job_with_ui_loop function.

Adding similar feature to Checkbox remote.

Make sure to keep existing feature, that is automatically add a
"Explicitly skipped before execution" comment when a non-cert-blocker
job is being skipped without any user-entered comment.

* Modify session resume behavior on Checkbox local

When a session is resumed, Checkbox (local mode) will ask the user what
to do.

If the previously run job is a cert-blocker, a behavior similar to a
normal job run is implemented: if the user wants to set the job outcome
to `skip` or `fail`, they will have to justify why by adding a comment.

In order to do so, a "Add a comment" choice is added, and a loop is run
until all the conditions are satisfied.

For non-cert-blockers, an automated comment is added depending on the
choice, except if a comment was manually entered.

* Cleanup plainbox job's certification status docstrings

* Add information about certification-status in Checkbox docs

The public doc has gone almost eight years (!) without a mention of the
`certification-status` option in the job unit.

This is now fixed.

In addition, a warning note is added to explain that cert-blockers
cannot be skipped or failed without the user providing a comment.

* Add Metabox scenarios for mandatory comments for cert-blocker jobs

These scenarios cover Checkbox local and test the following:

- A manual cert-blocker job cannot be failed until it has a comment
- A manual cert-blocker job cannot be skipped until it has a comment
- A user-interact-verify cert-blocker job cannot be failed until it has
a comment
- A user-interact-verify cert-blocker job cannot be skipped before being
run until it has a comment
- A user-interact-verify cert-blocker job cannot be skipped after being
run until it has a comment
- A user-interact cert-blocker job cannot be skipped before being run
until it has a comment
- After resuming a session, a manual cert-blocker job cannot be skipped
until it has a comment

* Modify wording of the warning when cert-blocker job is manually skipped/failed

checkbox-ng/checkbox_ng/launcher/master.py

@@ -539,6 +539,7 @@ def _maybe_manual_rerun_jobs(self):
 
     def _run_jobs(self, jobs_repr, total_num=0):
         for job in jobs_repr:
+            job_state = self.sa._sa.get_job_state(job['id'])
             SimpleUI.header(
                 _('Running job {} / {}').format(
                     job['num'], total_num,
@@ -584,7 +585,7 @@ def _run_jobs(self, jobs_repr, total_num=0):
                         job_lite = JobAdapter(job['command'])
                         try:
                             cmd = SimpleUI(None)._interaction_callback(
-                                job_lite, interaction.extra._builder)
+                                job_lite, job_state, interaction.extra._builder)
                             self.sa.remember_users_response(cmd)
                             self.finish_job(
                                 interaction.extra._builder.get_result())
@@ -601,10 +602,26 @@ def _run_jobs(self, jobs_repr, total_num=0):
                             _('Please enter your comments:') + '\n'))
                         self.sa.remember_users_response(new_comment + '\n')
                     elif interaction.kind == 'skip':
-                        self.finish_job(
-                            interaction.extra._builder.get_result())
-                        next_job = True
-                        break
+                        if (
+                            job_state.effective_certification_status == "blocker"
+                            and not isinstance(interaction.extra._builder.comments, str)
+                        ):
+                            print(self.C.RED(_("This job is required in order"
+                                               " to issue a certificate.")))
+                            print(
+                                self.C.RED(_("Please add a comment to explain"
+                                             " why you want to skip it."))
+                            )
+                            next_job = False
+                            self.sa.rerun_job(
+                                job['id'],
+                                interaction.extra._builder.get_result())
+                            break
+                        else:
+                            self.finish_job(
+                                interaction.extra._builder.get_result())
+                            next_job = True
+                            break
                 else:
                     self.wait_for_job()
                     break

checkbox-ng/checkbox_ng/launcher/stages.py

@@ -88,15 +88,15 @@ def _reset_auto_submission_retries(self):
 
 
     def _run_single_job_with_ui_loop(self, job, ui):
+        job_state = self.sa.get_job_state(job.id)
         print(self.C.header(job.tr_summary(), fill='-'))
         print(_("ID: {0}").format(job.id))
         print(_("Category: {0}").format(
-            self.sa.get_job_state(job.id).effective_category_id))
+            job_state.effective_category_id))
         comments = ""
         while True:
             if job.plugin in ('user-interact', 'user-interact-verify',
                               'user-verify', 'manual'):
-                job_state = self.sa.get_job_state(job.id)
                 if (not self.is_interactive and
                         job.plugin in ('user-interact',
                                        'user-interact-verify',
@@ -132,13 +132,30 @@ def _run_single_job_with_ui_loop(self, job, ui):
                             comments += new_comment + '\n'
                         continue
                     elif cmd == 'skip':
-                        result_builder = JobResultBuilder(
-                            outcome=IJobResult.OUTCOME_SKIP,
-                            comments=_("Explicitly skipped before"
-                                       " execution"))
-                        if comments != "":
-                            result_builder.comments = comments
-                        break
+                        if (
+                            job_state.effective_certification_status == "blocker"
+                            and comments == ""
+                        ):
+                            print(
+                                self.C.RED(
+                                    _("This job is required in order to issue a certificate.")
+                                )
+                            )
+                            print(
+                                self.C.RED(
+                                    _(
+                                        "Please add a comment to explain why you want to skip it."
+                                    )
+                                )
+                            )
+                            continue
+                        else:
+                            result_builder = JobResultBuilder(
+                                outcome=IJobResult.OUTCOME_SKIP,
+                                comments=_("Explicitly skipped before execution"))
+                            if comments != "":
+                                result_builder.comments = comments
+                            break
                     elif cmd == 'quit':
                         raise SystemExit()
                 else:
@@ -153,14 +170,14 @@ def _run_single_job_with_ui_loop(self, job, ui):
                     if comments != "":
                         result_builder.comments = comments
                     ui.notify_about_verification(job)
-                    self._interaction_callback(job, result_builder)
+                    self._interaction_callback(job, job_state, result_builder)
                 except ReRunJob:
                     self.sa.use_job_result(job.id, result_builder.get_result())
                     continue
             break
         return result_builder
 
-    def _interaction_callback(self, job, result_builder,
+    def _interaction_callback(self, job, job_state, result_builder,
                               prompt=None, allowed_outcome=None):
         result = result_builder.get_result()
         if prompt is None:
@@ -212,9 +229,33 @@ def _interaction_callback(self, job, result_builder,
             if cmd == 'set-pass':
                 result_builder.outcome = IJobResult.OUTCOME_PASS
             elif cmd == 'set-fail':
-                result_builder.outcome = IJobResult.OUTCOME_FAIL
+                if (
+                    job_state.effective_certification_status == "blocker"
+                    and not isinstance(result_builder.comments, str)
+                ):
+                    print(self.C.RED(_("This job is required in order to issue a certificate.")))
+                    print(
+                        self.C.RED(_("Please add a comment to explain why it failed."))
+                    )
+                    continue
+                else:
+                    result_builder.outcome = IJobResult.OUTCOME_FAIL
             elif cmd == 'set-skip' or cmd is None:
-                result_builder.outcome = IJobResult.OUTCOME_SKIP
+                if (
+                   job_state.effective_certification_status == "blocker"
+                   and not isinstance(result_builder.comments, str)
+                ):
+                    print(self.C.RED(_("This job is required in order to issue a certificate.")))
+                    print(
+                        self.C.RED(
+                            _(
+                                "Please add a comment to explain why you want to skip it."
+                            )
+                        )
+                    )
+                    continue
+                else:
+                    result_builder.outcome = IJobResult.OUTCOME_SKIP
             elif cmd == 'set-suggested':
                 result_builder.outcome = suggested_outcome
             elif cmd == 'set-comments':

checkbox-ng/checkbox_ng/launcher/subcommands.py

@@ -460,29 +460,69 @@ def _handle_last_job_after_resume(self, last_job):
 
         print(_("Previous session run tried to execute job: {}").format(
             last_job))
-        cmd = self._pick_action_cmd([
-            Action('s', _("skip that job"), 'skip'),
-            Action('p', _("mark it as passed and continue"), 'pass'),
-            Action('f', _("mark it as failed and continue"), 'fail'),
-            Action('r', _("run it again"), 'run'),
-        ], _("What do you want to do with that job?"))
-        if cmd == 'skip' or cmd is None:
-            result = MemoryJobResult({
-                'outcome': IJobResult.OUTCOME_SKIP,
-                'comments': _("Skipped after resuming execution")
-            })
-        elif cmd == 'pass':
-            result = MemoryJobResult({
-                'outcome': IJobResult.OUTCOME_PASS,
-                'comments': _("Passed after resuming execution")
-            })
-        elif cmd == 'fail':
-            result = MemoryJobResult({
-                'outcome': IJobResult.OUTCOME_FAIL,
-                'comments': _("Failed after resuming execution")
-            })
-        elif cmd == 'run':
-            result = None
+        last_job_cert_status = self.ctx.sa.get_job_state(last_job).effective_certification_status
+        result_dict = {
+            "outcome": IJobResult.OUTCOME_NONE,
+            "comments": ""
+        }
+        while True:
+            cmd = self._pick_action_cmd([
+                Action("c", _("add comment"), "comment"),
+                Action("s", _("skip that job"), "skip"),
+                Action("p", _("mark it as passed and continue"), "pass"),
+                Action("f", _("mark it as failed and continue"), "fail"),
+                Action("r", _("run it again"), "run"),
+            ], _("What do you want to do with that job?"))
+            if cmd == "skip" or cmd is None:
+                if (last_job_cert_status == "blocker" and not result_dict["comments"]):
+                    print(
+                        self.C.RED(_("This job is required in order to issue a certificate."))
+                    )
+                    print(
+                        self.C.RED(
+                            _(
+                                "Please add a comment to explain why you want to skip it."
+                            )
+                        )
+                    )
+                    continue
+                else:
+                    if not result_dict["comments"]:
+                        result_dict["comments"] = _("Skipped after resuming execution")
+                    result_dict["outcome"] = IJobResult.OUTCOME_SKIP
+                    result = MemoryJobResult(result_dict)
+                    break
+            elif cmd == "pass":
+                if not result_dict["comments"]:
+                    result_dict["comments"] = _("Passed after resuming execution")
+                result_dict["outcome"] = IJobResult.OUTCOME_PASS
+                result = MemoryJobResult(result_dict)
+                break
+            elif cmd == "fail":
+                if (last_job_cert_status == "blocker" and not result_dict["comments"]):
+                    print(
+                        self.C.RED(_("This job is required in order to issue a certificate."))
+                    )
+                    print(
+                        self.C.RED(_("Please add a comment to explain why it failed."))
+                    )
+                    continue
+                else:
+                    if not result_dict["comments"]:
+                        result_dict["comments"] = _("Failed after resuming execution")
+                    result_dict["outcome"] = IJobResult.OUTCOME_FAIL
+                    result = MemoryJobResult(result_dict)
+                    break
+            elif cmd == "comment":
+                new_comment = input(
+                    self.C.BLUE(_("Please enter your comments:") + "\n")
+                )
+                if new_comment:
+                    result_dict["comments"] += new_comment + "\n"
+                continue
+            elif cmd == "run":
+                result = None
+                break
         if result:
             self.ctx.sa.use_job_result(last_job, result)
 

checkbox-ng/docs/units/job.rst

@@ -59,6 +59,39 @@ Following fields may be used by the job unit:
           records, containing key/value pairs, and that can be used in other
           jobs' ``requires`` expressions.
 
+``certification-status``:
+    (optional) - Certification status for the given job. This is used by
+    Canonical to determine the jobs that **must** be run in order to be able to
+    issue a certificate. The allowed values are:
+
+    :unspecified:
+        This value means that a job was not analyzed in the context of
+        certification status classification and it has no classification at this
+        time. This is also the default certification status for all jobs.
+    :not-part-of-certification:
+        This value means that a given job may fail and this will not affect the
+        certification process in any way. Typically jobs with this certification
+        status are not executed during the certification process.
+    :non-blocker:
+        This value means that a given job may fail and while that should be
+        regarded as a possible future problem it will not block the
+        certification process. Canonical reserves the right to promote jobs from
+        *non-blocker* to *blocker*.
+    :blocker:
+        This value means that a given job **must** pass for the certification
+        process to succeed.
+
+    .. note::
+        The certification status can be overridden in a test plan.
+
+    .. warning::
+        If a job requiring user interaction (i.e. its ``plugin`` value is set to
+        ``manual``, ``user-interact`` or ``user-interact-verify``) has a
+        ``certification-status`` set to ``blocker``, it cannot be skipped or
+        failed unless the user provides a comment. This is so that the
+        Certification team can evaluate the test report and investigate the
+        reasons behind such an outcome.
+
 ``requires``:
     (optional). If specified, the job will only run if the conditions
     expressed in this field are met.

checkbox-ng/plainbox/impl/session/remote_assistant.py

@@ -441,10 +441,11 @@ def cant_start_builder(*args, **kwargs):
             if self._last_response == 'skip':
                 def skipped_builder(*args, **kwargs):
                     result_builder = JobResultBuilder(
-                        outcome=IJobResult.OUTCOME_SKIP,
-                        comments=_("Explicitly skipped before execution"))
+                        outcome=IJobResult.OUTCOME_SKIP)
                     if self._current_comments != "":
                         result_builder.comments = self._current_comments
+                    elif job_state.effective_certification_status != "blocker":
+                        result_builder.comments = "Explicitly skipped before execution"
                     return result_builder
                 self._be = BackgroundExecutor(
                     self, job_id, skipped_builder)

checkbox-ng/plainbox/impl/unit/job.py

@@ -121,29 +121,22 @@ class _CertificationStatusValues(SymbolDef):
     Particular values have the following meanings.
 
     unspecified:
-        One of the new possible certification status values. This value means
-        that a job was not analyzed in the context of certification status
-        classification and it has no classification at this time. This is also
-        the implicit certification status for all jobs.
+        This value means that a job was not analyzed in the context of
+        certification status classification and it has no classification at this
+        time. This is also the implicit certification status for all jobs.
     not-part-of-certification:
-        One of the new possible certification status values. This value means
-        that a given job may fail and this will not affect the certification
-        process in any way. Typically jobs with this certification status are
-        not executed during the certification process. In the past this was
-        informally referred to as a *blacklist item*.
+        This value means that a given job may fail and this will not affect the
+        certification process in any way. Typically jobs with this certification
+        status are not executed during the certification process.
     non-blocker:
-        One of the new possible certification status values. This value means
-        that a given job may fail and while that should be regarded as a
-        possible future problem it will not block the certification process. In
-        the past this was informally referred to as a *graylist item*.
-        Canonical reserves the right to promote jobs from the *non-blocker* to
-        *blocker*.
+        This value means that a given job may fail and while that should be
+        regarded as a possible future problem it will not block the
+        certification process. Canonical reserves the right to promote jobs from
+        *non-blocker* to *blocker*.
     blocker:
-        One of the new possible certification status values. This value means
-        that a given job must pass for the certification process to succeed. In
-        the past this was informally referred to as a *whitelist item*. The
-        term *blocker* was chosen to disambiguate the meaning of the two
-        concepts.
+        This value means that a given job must pass for the certification
+        process to succeed. The term *blocker* was chosen to disambiguate the
+        meaning of the two concepts.
     """
     unspecified = 'unspecified'
     not_part_of_certification = 'not-part-of-certification'

metabox/metabox/metabox-provider/units/cert-blocker-tps.pxu

@@ -0,0 +1,35 @@
+id: cert-blocker-manual
+unit: test plan
+_name: Manual cert-blocker test
+_description:
+    Test that a cert-blocker manual job cannot be skipped or failed without a
+    comment.
+include:
+    stub/split-fields/manual certification-status=blocker
+
+id: cert-blocker-user-interact-verify
+unit: test plan
+_name: User interact verify cert-blocker test
+_description:
+    Test that cert-blocker user-interact-verify job cannot be skipped or failed
+    without a comment.
+include:
+    stub/split-fields/user-interact-verify certification-status=blocker
+
+id: cert-blocker-user-interact
+unit: test plan
+_name: User interact verify cert-blocker test
+_description:
+    Test that cert-blocker user-interact job cannot be skipped without a comment.
+include:
+    stub/split-fields/user-interact certification-status=blocker
+
+id: cert-blocker-manual-resume
+unit: test plan
+_name: Manual cert-blocker test
+_description:
+    Test that a job marked as cert-blocker cannot be skipped without a comment
+    when resuming a session
+include:
+    stub/split-fields/manual certification-status=blocker
+    stub/split-fields/user-interact