Skip to content
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

Allow to email users from a management command #8243

Merged
merged 13 commits into from Jul 7, 2021
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jun 8, 2021

Now we can email all owners from a management command. If we need to email other type of users we can change the users queryset, or we could also implement this with eval to make it more dynamic p:

@stsewd stsewd requested a review from ericholscher June 8, 2021 19:25
@stsewd stsewd marked this pull request as ready for review June 8, 2021 19:25
readthedocs/settings/base.py Outdated Show resolved Hide resolved
readthedocs/settings/base.py Outdated Show resolved Hide resolved
parser.add_argument(
'--notification',
help='Path to a file with the notification content in markdown.',
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to be able to pass an --organization or --project here, so we can scope things a bit. We rarely want to send emails to every organization, as much as a subset with some status.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, had to add a new filter for owners in our AdminPermission class.

log = logging.getLogger(__name__)


class Command(BaseCommand):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a docstring showing how to use it.

Is there a good way to send arbitrary notifications? Or is everything required to be set in a template?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to put this in the --help option, but looks like it's just a one-line description. I have added some examples in the docstring.

Is there a good way to send arbitrary notifications? Or is everything required to be set in a template?

Not sure what you mean, we can send notifications with

contact_owners --notification notification.md

notication.md will be parsed through markdown first.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me 👍

@stsewd stsewd enabled auto-merge (squash) July 7, 2021 14:55
@stsewd stsewd merged commit 2bd2048 into master Jul 7, 2021
@stsewd stsewd deleted the move-email-users branch July 7, 2021 15:23
Comment on lines +16 to +17
If organizations are enabled,
we return the owners of the project organization instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you consider owners of the project to be the OrganizationOwners instead of the users that have ADMIN permissions over that project? I mean, members of admin Teams that include this project.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admins don't have all permissions to change settings from the org level. contact_users can be called manually from the django admin if we need to contact other type of users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I'm talking at a higher level, not for this particular case to contact users, but for the usage of AdminPermission.owner in general in other contexts as well.

With this implementation AdminPermission.owners(project) and AdminPermission.owners(organization) both return the same set of users and I think that's wrong. To me, AdminPermission.owners(project) should return users that have admin permissions in that particular project since we are asking for the "owners of a project" and being an owner of a project does not require to have permissions to change settings at the organization level.

In fact, thinking more about this AdminPermission.owners does exactly the same as the method we already had: AdminPermission.admin. So, we should probably delete .owners and just .admins instead of passing project or organization depending on the case required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants