Implement cli recurrence update (#51)

- **Make cli.update test more flexible**
- **Client Bugfix: Actually update the payload**
- **Add CLI support for updating the recurrence**

Author: Marenz

RELEASE_NOTES.md

@@ -14,4 +14,4 @@
 
 ## Bug Fixes
 
-<!-- Here goes notable bug fixes that are worth a special mention or explanation -->
+* Fixed that client.update() was ignoring updates to "payload".

src/frequenz/client/dispatch/__main__.py

@@ -156,6 +156,73 @@ def validate_reccurance(ctx: click.Context, param: click.Parameter, value: Any)
     return value
 
 
+recurrence_options: list[click.Parameter] = [
+    click.Option(
+        ["--frequency", "-f"],
+        type=click.Choice(
+            [
+                frequency.name
+                for frequency in Frequency
+                if frequency != Frequency.UNSPECIFIED
+            ],
+            case_sensitive=False,
+        ),
+        help="Frequency of the dispatch",
+        callback=validate_reccurance,
+        is_eager=True,
+    ),
+    click.Option(
+        ["--interval"],
+        type=int,
+        help="Interval of the dispatch, based on frequency",
+        default=0,
+    ),
+    click.Option(
+        ["--count"],
+        type=int,
+        help="Number of occurrences of the dispatch",
+        callback=validate_reccurance,
+    ),
+    click.Option(
+        ["--until"],
+        type=FuzzyDateTime(),
+        help="End time of the dispatch",
+        callback=validate_reccurance,
+    ),
+    click.Option(
+        ["--by-minute"],
+        type=int,
+        help="Minute of the hour for the dispatch",
+        multiple=True,
+        callback=validate_reccurance,
+    ),
+    click.Option(
+        ["--by-hour"],
+        type=int,
+        help="Hour of the day for the dispatch",
+        multiple=True,
+        callback=validate_reccurance,
+    ),
+    click.Option(
+        ["--by-weekday"],
+        type=click.Choice(
+            [weekday.name for weekday in Weekday if weekday != Weekday.UNSPECIFIED],
+            case_sensitive=False,
+        ),
+        help="Day of the week for the dispatch",
+        multiple=True,
+        callback=validate_reccurance,
+    ),
+    click.Option(
+        ["--by-monthday"],
+        type=int,
+        help="Day of the month for the dispatch",
+        multiple=True,
+        callback=validate_reccurance,
+    ),
+]
+
+
 @cli.command()
 @click.argument("microgrid-id", required=True, type=int)
 @click.argument(
@@ -172,70 +239,6 @@ def validate_reccurance(ctx: click.Context, param: click.Parameter, value: Any)
     "--payload", "-p", type=JsonDictParamType(), help="JSON payload for the dispatch"
 )
 @click.pass_context
-@click.option(
-    "--frequency",
-    "-f",
-    type=click.Choice(
-        [
-            frequency.name
-            for frequency in Frequency
-            if frequency != Frequency.UNSPECIFIED
-        ],
-        case_sensitive=False,
-    ),
-    help="Frequency of the dispatch",
-    callback=validate_reccurance,
-    is_eager=True,
-)
-@click.option(
-    "--interval",
-    type=int,
-    help="Interval of the dispatch, based on frequency",
-    default=0,
-)
-@click.option(
-    "--count",
-    type=int,
-    help="Number of occurrences of the dispatch",
-    callback=validate_reccurance,
-)
-@click.option(
-    "--until",
-    type=FuzzyDateTime(),
-    help="End time of the dispatch",
-    callback=validate_reccurance,
-)
-@click.option(
-    "--by-minute",
-    type=int,
-    help="Minute of the hour for the dispatch",
-    multiple=True,
-    callback=validate_reccurance,
-)
-@click.option(
-    "--by-hour",
-    type=int,
-    help="Hour of the day for the dispatch",
-    multiple=True,
-    callback=validate_reccurance,
-)
-@click.option(
-    "--by-weekday",
-    type=click.Choice(
-        [weekday.name for weekday in Weekday if weekday != Weekday.UNSPECIFIED],
-        case_sensitive=False,
-    ),
-    help="Day of the week for the dispatch",
-    multiple=True,
-    callback=validate_reccurance,
-)
-@click.option(
-    "--by-monthday",
-    type=int,
-    help="Day of the month for the dispatch",
-    multiple=True,
-    callback=validate_reccurance,
-)
 async def create(
     ctx: click.Context,
     /,
@@ -269,20 +272,36 @@ async def create(
 @click.option("--duration", type=FuzzyTimeDelta())
 @click.option("--selector", type=SelectorParamType())
 @click.option("--active", type=bool)
+@click.option(
+    "--payload", "-p", type=JsonDictParamType(), help="JSON payload for the dispatch"
+)
 @click.pass_context
 async def update(
     ctx: click.Context, dispatch_id: int, **new_fields: dict[str, Any]
 ) -> None:
     """Update a dispatch."""
-    # Remove keys with `None` value from new_fields
-    new_fields = {k: v for k, v in new_fields.items() if v is not None}
+
+    def skip_field(value: Any) -> bool:
+        return value is None or value == [] or value == ()
+
+    # Every field is initialized with `None`, repeatable ones with `()` and `[]`.
+    # We want to filter these out to not send them to the server.
+    new_fields = {k: v for k, v in new_fields.items() if not skip_field(v)}
+    recurrence = parse_recurrence(new_fields)
+
+    # Convert recurrence fields to nested fields as expected by update()
+    for key in recurrence.__dict__ if recurrence else []:
+        val = getattr(recurrence, key)
+        if val is not None and val != []:
+            new_fields[f"recurrence.{key}"] = val
 
     if len(new_fields) == 0:
         raise click.BadArgumentUsage("At least one field must be given to update.")
 
     try:
         await ctx.obj["client"].update(dispatch_id=dispatch_id, new_fields=new_fields)
-        click.echo("Dispatch updated.")
+        click.echo("Dispatch updated:")
+        click.echo(pformat(await ctx.obj["client"].get(dispatch_id), compact=True))
     except grpc.RpcError as e:
         raise click.ClickException(f"Update failed: {e}")
 
@@ -378,6 +397,12 @@ async def display_help() -> None:
                 click.echo(e)
 
 
+# Add recurrence options to the create command
+create.params += recurrence_options  # pylint: disable=no-member
+# Add recurrence options to the update command
+update.params += recurrence_options  # pylint: disable=no-member
+
+
 def main() -> None:
     """Entrypoint for the CLI."""
     if len(sys.argv) > 1:

src/frequenz/client/dispatch/_client.py

@@ -211,6 +211,8 @@ async def update(
                     msg.update.selector.CopyFrom(component_selector_to_protobuf(val))
                 case "is_active":
                     msg.update.is_active = val
+                case "payload":
+                    msg.update.payload.update(val)
                 case "active":
                     msg.update.is_active = val
                     key = "is_active"

tests/test_dispatch_cli.py

@@ -356,7 +356,7 @@ async def test_create_command(  # pylint: disable=too-many-arguments,too-many-lo
 
 @pytest.mark.asyncio
 @pytest.mark.parametrize(
-    "dispatches, field, value, update_field, update_value, expected_return_code, expected_output",
+    "dispatches, args, fields, expected_return_code, expected_output",
     [
         (
             [
@@ -375,12 +375,13 @@ async def test_create_command(  # pylint: disable=too-many-arguments,too-many-lo
                     update_time=datetime(2023, 1, 1, 0, 0, 0),
                 )
             ],
-            "--duration",
-            "7200",
-            "duration",
-            timedelta(seconds=7200),
+            [
+                "--duration",
+                "7200",
+            ],
+            {"duration": timedelta(seconds=7200)},
             0,
-            "Dispatch updated.",
+            "duration=datetime.timedelta(seconds=7200)",
         ),
         (
             [
@@ -399,12 +400,15 @@ async def test_create_command(  # pylint: disable=too-many-arguments,too-many-lo
                     update_time=datetime(2023, 1, 1, 0, 0, 0),
                 )
             ],
-            "--active",
-            "False",
-            "active",
-            False,
+            [
+                "--active",
+                "False",
+            ],
+            {
+                "active": False,
+            },
             0,
-            "Dispatch updated.",
+            "active=False",
         ),
         (
             [
@@ -423,19 +427,64 @@ async def test_create_command(  # pylint: disable=too-many-arguments,too-many-lo
                     update_time=datetime(2023, 1, 1, 0, 0, 0),
                 )
             ],
-            "--selector",
-            "400, 401",
-            "selector",
-            [400, 401],
+            [
+                "--selector",
+                "400, 401",
+                "--frequency",
+                "daily",
+                "--interval",
+                "5",
+                "--count",
+                "10",
+                "--by-minute",
+                "0",
+                "--by-minute",
+                "3",
+                "--by-minute",
+                "5",
+                "--by-minute",
+                "30",
+                "--payload",
+                '{"key": "value"}',
+            ],
+            {
+                "selector": [400, 401],
+                "recurrence": RecurrenceRule(
+                    frequency=Frequency.DAILY,
+                    interval=5,
+                    end_criteria=EndCriteria(
+                        count=10,
+                        until=None,
+                    ),
+                    byminutes=[0, 3, 5, 30],
+                    byhours=[],
+                    byweekdays=[],
+                    bymonthdays=[],
+                ),
+                "payload": {"key": "value"},
+            },
             0,
-            "Dispatch updated.",
+            """         selector=[400, 401],
+         active=True,
+         dry_run=False,
+         payload={'key': 'value'},
+         recurrence=RecurrenceRule(frequency=<Frequency.DAILY: 3>,
+                                   interval=5,
+                                   end_criteria=EndCriteria(count=10,
+                                                            until=None),
+                                   byminutes=[0, 3, 5, 30],
+                                   byhours=[],
+                                   byweekdays=[],
+                                   bymonthdays=[],
+                                   bymonths=[]),""",
         ),
         (
             [],
-            "--duration",
-            "frankly my dear, I don't give a damn",
-            "",
-            None,
+            [
+                "--duration",
+                "frankly my dear, I don't give a damn",
+            ],
+            {},
             2,
             "Error: Invalid value for '--duration': Could not parse time expression",
         ),
@@ -445,20 +494,20 @@ async def test_update_command(  # pylint: disable=too-many-arguments
     runner: CliRunner,
     fake_client: FakeClient,
     dispatches: list[Dispatch],
-    field: str,
-    value: str,
-    update_field: str,
-    update_value: Any,
+    args: list[str],
+    fields: dict[str, Any],
     expected_return_code: int,
     expected_output: str,
 ) -> None:
     """Test the update command."""
     fake_client.dispatches = dispatches
-    result = await runner.invoke(cli, ["update", "1", field, value])
-    assert result.exit_code == expected_return_code
+    result = await runner.invoke(cli, ["update", "1", *args])
     assert expected_output in result.output
-    if expected_return_code == 0:
-        assert getattr(fake_client.dispatches[0], update_field) == update_value
+    assert result.exit_code == expected_return_code
+    if dispatches:
+        assert len(fake_client.dispatches) == 1
+        for key, value in fields.items():
+            assert getattr(fake_client.dispatches[0], key) == value
 
 
 @pytest.mark.asyncio