Merge pull request #237 from cleverage/234

#234 - Notifications: a process stopped by a StopTask with reported log levels triggers finish_with_report

Author: njoubert-cleverage

CHANGELOG.md

@@ -5,6 +5,7 @@ Latest
 * [#228](https://github.com/cleverage/ui-process-bundle/issues/228) `ProcessExecuteHandler`, `ProcessExecuteController` (`cleverage_ui_process_http_execute`): only the files written by the launch form or the HTTP API for the execution are deleted (#209), flagged by the new `deleteInput` argument of `ProcessExecuteMessage` (`false` by default): a file of the upload directory given by its path (HTTP API, text input of the launch form, schedule, default input) is no longer deleted. A message failing in a Messenger worker keeps its file while it will be retried or when it is sent to a failure transport (`messenger:failed:retry`), new internal `UploadedFileSubscriber`. The file uploaded with a request rejected by the HTTP API (422) is deleted. The messages queued by 3.4 have no flag: their file is no longer deleted by the worker, it may need a manual cleanup of the upload directory (`cleverage_ui_process.upload_directory`). Update documentation, add tests.
 * [#229](https://github.com/cleverage/ui-process-bundle/issues/229) Login throttling of the `main` firewall (#207, #222): the requests of the HTTP API (`HttpProcessExecutionAuthenticator`) are no longer counted, on every firewall with a login throttling (`LoginThrottlingPass` decorates its limiter, the firewall settings still apply to the login form): failed logins blocked a valid API token, and invalid tokens blocked the login form of the IP address. `login_throttling: false` in the application `main` firewall disables it (it was enabled again by the bundle). With `framework.rate_limiter: false`, an explicit error tells how to fix the configuration instead of "Parent definition "limiter" does not exist". `HttpProcessExecutionAuthenticator::onAuthenticationFailure()` answers the message key of the exception, instead of an empty message when there were too many attempts. Update documentation, add tests.
 * [#230](https://github.com/cleverage/ui-process-bundle/issues/230) `DoctrineProcessHandler`, `LogRecord`: a log record with an invalid UTF-8 sequence in its message or its context (e.g. a Latin-1 file imported), or with a message longer than the column in code points (`LogRecord` truncated it to 512 graphemes: emojis, combining marks), no longer makes the insertion of the logs in database fail: the invalid sequences are replaced (and the NUL characters of the message removed) and the message is truncated to 512 code points. When the insertion of the log records fails anyway, they (up to 500) are dropped and the failure is logged on the `cleverage_process` channel, instead of making the process fail (on the 500th record, even with `error_strategy: skip`) or preventing the status of its execution from being saved at its end (left `started`, the command exiting with `1`); a failure of a flush also writing pending changes of the process tasks (the entity manager is shared) is not caught, as before. Update documentation, add tests.
+* [#234](https://github.com/cleverage/ui-process-bundle/issues/234) Notifications: a process execution stopped on purpose by a `StopTask` with log levels counted in its report (`logs.report_increment_level`) triggers `finish_with_report`, notified by default, instead of `stopped`, not notified by default (#220): a process logging errors then stopping on purpose is notified again, as before 3.4. Without such log levels, or when stopped from the UI, it still triggers `stopped`. Update documentation, add tests.
 
 v3.4
 ------

UPGRADE.md

@@ -1,6 +1,14 @@
 Upgrade Guide
 =============
 
+## v3.4.1
+
+### Processes stopped by a `stop` task
+
+A process stopped on purpose by a `stop` task with log levels counted in its report (`logs.report_increment_level`) is
+notified as `finish_with_report` (notified by default) instead of `stopped`. If you added `stopped` to the notified
+`statuses` after upgrading to 3.4, you can remove it: only the stops without such log levels are left.
+
 ## v3.4
 
 No deprecation in this version (the `$translator` argument of `ProcessExecutionCrudController::__construct()` triggers

docs/reference/01-bundle_configuration.md

@@ -77,7 +77,7 @@ Notification of the end of the process executions, with [symfony/notifier](https
 | Key          | Type       | Default                          | Description                                                                                                                         |
 |--------------|------------|----------------------------------|-------------------------------------------------------------------------------------------------------------------------------------|
 | `enabled`    | `bool`     | `false`                          | Notify the end of the process executions. `true` requires `symfony/notifier`.                                                       |
-| `statuses`   | `string[]` | `[failed, finish_with_report]`   | Ends of process executions to notify: `failed`, `finish_with_report` (finished with log levels counted in its report), `finish` (finished without them), `stopped` (stopped from the UI). |
+| `statuses`   | `string[]` | `[failed, finish_with_report]`   | Ends of process executions to notify: `failed`, `finish_with_report` (finished, or stopped by a `StopTask`, with log levels counted in its report), `finish` (finished without them), `stopped` (stopped from the UI, or by a `StopTask` without them). |
 | `channels`   | `string[]` | `[]`                             | Notifier channels, e.g. `chat/slack`, `email`. Empty: the `channel_policy` of the notifier, by importance.                           |
 | `recipients` | `array[]`  | `[]`                             | Recipients, with an `email` and/or a `phone`. Empty: the `admin_recipients` of the notifier.                                         |
 

docs/reference/09-notifications.md

@@ -85,9 +85,9 @@ Statuses
 | Status               | End of the process execution                                                                                                       | Importance |
 |----------------------|------------------------------------------------------------------------------------------------------------------------------------|------------|
 | `failed`             | Failed (status `failed`).                                                                                                          | `high`     |
-| `finish_with_report` | Finished (status `finish`) with log levels counted in its [report](04-process_executions_and_logs.md#report), e.g. `{"Warning": 3}`. The counted levels depend on `logs.report_increment_level` (default `Warning`). | `medium`   |
+| `finish_with_report` | Finished (status `finish`) with log levels counted in its [report](04-process_executions_and_logs.md#report), e.g. `{"Warning": 3}`, or stopped on purpose by a `StopTask` of the process (status `stopped`) with such log levels. The counted levels depend on `logs.report_increment_level` (default `Warning`). | `medium`   |
 | `finish`             | Finished without log levels counted in its report.                                                                                | `low`      |
-| `stopped`            | Stopped from the UI (status `stopped`, see [stopping a process execution](04-process_executions_and_logs.md#stopping-a-process-execution)) or on purpose by a `StopTask` of the process. Not notified by default. | `low`      |
+| `stopped`            | Stopped from the UI (status `stopped`, see [stopping a process execution](04-process_executions_and_logs.md#stopping-a-process-execution)), or on purpose by a `StopTask` of the process without log levels counted in its report. Not notified by default. | `low`      |
 
 The importance selects the channels of the notifier `channel_policy` when no `channels` are configured.
 

src/Notifier/NotificationTrigger.php

@@ -23,18 +23,22 @@
 enum NotificationTrigger: string
 {
     case Failed = 'failed';
-    /** Finished with log levels counted in its report (see the "logs.report_increment_level" configuration) */
+    /**
+     * Finished with log levels counted in its report (see the "logs.report_increment_level" configuration), or stopped
+     * on purpose by a StopTask with such log levels.
+     */
     case FinishWithReport = 'finish_with_report';
     case Finish = 'finish';
-    /** Stopped on a stop request from the UI, or on purpose by a StopTask */
+    /** Stopped on a stop request from the UI, or on purpose by a StopTask without log levels counted in its report */
     case Stopped = 'stopped';
 
     public static function fromProcessExecution(ProcessExecution $processExecution): ?self
     {
         return match ($processExecution->status) {
             ProcessExecutionStatus::Failed => self::Failed,
             ProcessExecutionStatus::Finish => [] === self::getReportedLevels($processExecution) ? self::Finish : self::FinishWithReport,
-            ProcessExecutionStatus::Stopped => self::Stopped,
+            // A stop request ends with a process failure, counted in the report: only the StopTask reports matter
+            ProcessExecutionStatus::Stopped => $processExecution->isStopRequested() || [] === self::getReportedLevels($processExecution) ? self::Stopped : self::FinishWithReport,
             ProcessExecutionStatus::Started => null,
         };
     }

tests/Notifier/NotificationTriggerTest.php

@@ -26,14 +26,17 @@
 class NotificationTriggerTest extends TestCase
 {
     /**
-     * @return iterable<string, array{ProcessExecutionStatus, array<string, mixed>, ?NotificationTrigger}>
+     * @return iterable<string, array{0: ProcessExecutionStatus, 1: array<string, mixed>, 2: ?NotificationTrigger, 3?: bool}>
      */
     public static function provideProcessExecutions(): iterable
     {
         yield 'started' => [ProcessExecutionStatus::Started, [], null];
         yield 'failed' => [ProcessExecutionStatus::Failed, [], NotificationTrigger::Failed];
         yield 'failed with report' => [ProcessExecutionStatus::Failed, ['Error' => 2], NotificationTrigger::Failed];
-        yield 'stopped' => [ProcessExecutionStatus::Stopped, ['Warning' => 1], NotificationTrigger::Stopped];
+        yield 'stopped' => [ProcessExecutionStatus::Stopped, [], NotificationTrigger::Stopped];
+        yield 'stopped with a custom report key' => [ProcessExecutionStatus::Stopped, ['imported' => 0], NotificationTrigger::Stopped];
+        yield 'stopped with report' => [ProcessExecutionStatus::Stopped, ['Error' => 2], NotificationTrigger::FinishWithReport];
+        yield 'stopped on a stop request' => [ProcessExecutionStatus::Stopped, ['Critical' => 1], NotificationTrigger::Stopped, true];
         yield 'finish' => [ProcessExecutionStatus::Finish, [], NotificationTrigger::Finish];
         yield 'finish with a custom report key' => [ProcessExecutionStatus::Finish, ['imported' => 12], NotificationTrigger::Finish];
         yield 'finish with report' => [ProcessExecutionStatus::Finish, ['Warning' => 3, 'imported' => 12], NotificationTrigger::FinishWithReport];
@@ -43,10 +46,13 @@ public static function provideProcessExecutions(): iterable
      * @param array<string, mixed> $report
      */
     #[DataProvider('provideProcessExecutions')]
-    public function testFromProcessExecution(ProcessExecutionStatus $status, array $report, ?NotificationTrigger $expected): void
+    public function testFromProcessExecution(ProcessExecutionStatus $status, array $report, ?NotificationTrigger $expected, bool $stopRequested = false): void
     {
         $processExecution = new ProcessExecution('test.process', 'test.log');
         $processExecution->setStatus($status);
+        if ($stopRequested) {
+            $processExecution->requestStop();
+        }
         foreach ($report as $key => $value) {
             $processExecution->addReport($key, $value);
         }