-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify the command dds motd send to allow the option to send only to unit personnel #717
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
This is the same as the other PR. I will review after merge this time, but as I said I want to review tasks that have a direct affect on the users. This time I was finishing up a report before I could review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a task to the product backlog regarding my comments in this PR. But also, did you check that codecov said that all lines were covered? I can't see the codecov
report in this PR, so I checked the github action regarding codecov and:
[2024-10-03T09:44:34.097Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 429 - {"message":"Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected time to availability: 442s."}
@click.option( | ||
"--unit-personnel-only", | ||
is_flag=True, | ||
required=False, | ||
default=False, | ||
help="Send MOTD to unit personnel only.", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather that this option is --unit-only
:
--unit-personnel-only
partly sounds like it's onlyUnit Personnel
who gets the email (Unit Admins should also get the email). This could cause confusion at some point.--unit-personnel-only
is imo unnecessarily long
Read this before submitting the PR
If there is a field which you are unsure about, enter the edit mode of this description or go to the PR template; There are invisible comments providing descriptions which may be of help.
1. Description / Summary
Modify the client to accept the option to send only to unit persoonel
2. Jira task / GitHub issue
Link to the github issue or add the Jira task ID here.
3. Type of change
What type of change(s) does the PR contain?
Check the relevant boxes below. For an explanation of the different sections, enter edit mode of this PR description template.
4. Additional information
master
branch: If checked, read the release instructions5. Actions / Scans
Check the boxes when the specified checks have passed.
For information on what the different checks do and how to fix it if they're failing, enter edit mode of this description or go to the PR template.