feat: add support for templates delete command
#82
Conversation
rlueder
force-pushed
the
main
branch
3 times, most recently
from
6dd7fbf
to
0f3ba45
Compare
rlueder
changed the title
templates delete command
|
@derekrushforth @tomek-ac just checking in on this PR, I used it again a couple of weeks ago and it worked pretty well. Would love to have this be part of the library instead of having to run my own fork. Let me know if you need anything else to get this merged. Thanks! |
There was a problem hiding this comment.
Thanks for working on this @rlueder! I left a few suggestions for you to consider.
Besides that here is a few other things that would need to be addressed before merging:
- the delete command can be run in non-interactive mode in CI/CD
- we have integration tests for this code
- the "delete" command is listed when running
postmark templates - we will need to update our wiki to explain how to use it. For this please prepare a separate Markdown file as part of the PR.
| /** | ||
| * Ask user a series of questions before deleting templates. | ||
| */ | ||
|
|
| /** | ||
| * if the user has selected to delete templates by id | ||
| */ | ||
|
|
There was a problem hiding this comment.
| /** | |
| * if the user has selected to delete templates by id | |
| */ |
This doesn't add much, the code is self-explanatory.
| // TODO we probably want an actual validation | ||
| // against ids/aliases that exist in the server |
There was a problem hiding this comment.
I'd say we don't need to validate that. Ideally running the command with a non-existing ID should display a warning.
| /** | ||
| * if the user has selected to delete all templates | ||
| */ | ||
|
|
There was a problem hiding this comment.
| /** | |
| * if the user has selected to delete all templates | |
| */ |
again, the code is self-explanatory. Comments are most valuable when they explain the "why" leaving the "what" to the code.
| const userInput = await input({ | ||
| message: | ||
| "Enter template id(s)/alias(es) - separate with commas if multiple:", | ||
| validate: validateUserInput, |
There was a problem hiding this comment.
Maybe we could inline the validation if it's so simple?
| validate: validateUserInput, | |
| validate: (s) => s.trim().length > 0, // don't allow empty input |
| if (templateType) { | ||
| confirmed = await confirm({ | ||
| default: false, | ||
| message: `Delete ALL templates? Are you sure?`, |
There was a problem hiding this comment.
| message: `Delete ALL templates? Are you sure?`, | |
| message: 'Delete ALL templates? Are you sure?', |
|
|
||
| if (!totalCount) { | ||
| spinner.stop(); | ||
| return fatalError("There are no templates on this server."); |
There was a problem hiding this comment.
We probably shouldn't exit with an error if there are no templates on the server. I guess this would only happen if the user requested to delete all the servers. In such case the user's intention is to not have templates so when there are not any, then we should be good.
| const response = await client.getTemplates({ | ||
| count: 300, | ||
| templateType, | ||
| }); |
There was a problem hiding this comment.
In 99% of the cases we would get all the templates with this call as Postmark allows for 100 templates per Server by default. But to make it a bit more resilient and correct, we should fetch all the templates, not the only first batch. We'd need a loop to fill the templates array.
|
|
||
| try { | ||
| let templates = []; | ||
| let totalCount = 0; |
There was a problem hiding this comment.
Can't we use templates.length instead of adding another variable?
| /* eslint-disable @typescript-eslint/member-delimiter-style */ | ||
|
|
There was a problem hiding this comment.
Please follow the local conventions.
| /* eslint-disable @typescript-eslint/member-delimiter-style */ |
This is an implementation of a
postmark templates deletecommand, allowing users to delete by id or alias (one or multiple) or all templates from their server.