WPA-14407: Added CLI support for panel based email services - #212
WPA-14407: Added CLI support for panel based email services#212sgimpel-godaddy wants to merge 1 commit into
Conversation
|
|
||
| | | | | ||
| |---|---| | ||
| | Flags | `--email <email>` (required), `--account-id`, `--first-name`, `--last-name`, repeatable `--consent <agreementType>` | |
There was a problem hiding this comment.
What is account ID?
|
|
||
| **Recommendation: native `--page`/`--page-size` flags, forwarded 1:1 to the | ||
| API's query params**, rather than adopting `--limit`/`--offset` via | ||
| `PaginationConfig`. Translating page-based server pagination into offset math |
There was a problem hiding this comment.
I'm pretty hesitant to have mismatching pagination styles. I think that --limit maps pretty cleanly to --page-size, and if --offset is a multiple of --page-size, then it should typically work the same (worst case scenario is that you have to fetch two pages and then slice them up to fit the provided offset/limit). We can specify default limits now, so the assertion that the handler must return the entire list isn't accurate (maybe that was based on an older version of the engine since enhanced pagination functionality is now available). We should be able to, for this list command have a default page size/limit, and if we wanted to add additional constraints around --limit and --offset (like forcing them to be page-aligned), that'd be better than having completely different pagination flags.
Worth discussing with the others.
| } | ||
|
|
||
| fn first_eligible_account_id(data: &Value) -> Option<String> { | ||
| data.get("eligibleAccounts")? |
There was a problem hiding this comment.
Not blocking, but I think a good backlog enhancement would be to switch this client to being spec-generated so that we have strongly-typed structures and avoid accidental data shape mismatches.
| struct CreateArgs { | ||
| #[arg(long, value_name = "EMAIL")] | ||
| email: String, | ||
| #[arg(long = "account-id", value_name = "ACCOUNT_ID")] |
There was a problem hiding this comment.
We should have the /// comments over all these arg so we can have better help text. I think --account-id especially needs an explanation.
|
I'd recommend attaching a markdown guide to the command module as well, just to explain the email system, things like what is an account ID, etc. |
2 participants
No description provided.