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

Add invite event host button on user management page #2123

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

mscanlan-git
Copy link
Contributor

This PR partially addresses #2104 by adding a button to the user management page that is able to add a user to the event host permission group. I also added a pop-up after clicking the button to ensure the admin is committed to adding the user as an event host.

Two additional changes I plan to make to follow-up on this:

  • Add a 'remove' event host button for circumstances where we need to remove someone's permissions
  • Add an email option on click which will send instructions on the events functionality to the user

Adds invite event host functionality button

temp
@bemoody
Copy link
Collaborator

bemoody commented Nov 2, 2023

Thanks for working on this! There are a couple of problems I see with the implementation here:

  1. When someone submits a form, you always need to check that the request method was POST. (The only exception is a "search" form, which can use the GET method because it doesn't have any lasting effect on the server.)

Think of it this way: if it's a POST request, we know that the form was actually submitted by the given user (request.user). Other types of HTTP requests do not have that guarantee, and can easily be spoofed.

So any view that does a save or create or delete, or anything like that, always needs to check if request.method == 'POST' before doing so. You can find lots of examples of this throughout the code.

  1. We only want to allow certain authorized people to add event hosts, which is why we want the @permission_required decorator here. But I think the permission should be something more specific than user.view_all_events. I'm not sure if any of the existing permissions are suitable or if we should add a new permission to cover this case.

@mscanlan-git
Copy link
Contributor Author

Thanks for your feedback, @bemoody.

1. When someone submits a form, you **always need** to check that the request method was POST.  (The only exception is a "search" form, which can use the GET method because it doesn't have any lasting effect on the server.)

I have since added this to the view.

user.view_all_events. I'm not sure if any of the existing permissions are suitable or if we should add a new permission to cover this case.

I'm not sure either, it may be worth adding a new permission? I mainly copied the permission from the event_management function as it seemed the most relatable permission group to assign to this function.

@tompollard
Copy link
Member

@mscanlan-git I think we probably want to create a permission specifically for the ability to invite hosts. You could do this by adding the permission to the Event model, e.g.:

class Event(models.Model):
    """
    Captures information on events such as datathons, workshops and classes.
    Used to allow event hosts to assist with credentialing.
    """
    title = models.CharField(max_length=128)
    description = SafeHTMLField(max_length=10000, blank=True, null=True)
    category = models.CharField(choices=EventCategory.choices, max_length=32)
    host = models.ForeignKey("user.User", on_delete=models.CASCADE)
    added_datetime = models.DateTimeField(auto_now_add=True)
    start_date = models.DateField(default=timezone.now)
    end_date = models.DateField(default=timezone.now)
    slug = models.SlugField(unique=True)
    allowed_domains = models.CharField(blank=True, null=True, validators=[
                                       validators.validate_domain_list], max_length=100)

    class Meta:
        unique_together = ('title', 'host')
        permissions = [('view_all_events', 'Can view all events in the console'),
                       ('invite_event_host', 'Can grant event host status to a user'),
                       ('view_event_menu', 'Can view event menu in the navbar')]

Some other notes:

  • After modifying the permission, you would need to generate a migration with python manage.py makemigrations and then apply the migration with python manage.py migrate.
  • For testing, you can assign the invite_event_host permission to a user from the console at https://physionet.org/admin/user/user/ (log in as admin).
  • We would need to modify the "Invite event host" button to make it clear who can and cannot use it (e.g. deactivate it or hide it for people without the invite_event_host permission?
  • Currently the invite_event_host is displayed and active for users who are already event hosts. What is the desired behaviour in this situation?

@mscanlan-git
Copy link
Contributor Author

mscanlan-git commented Nov 27, 2023

Thanks @tompollard, many of these changes have been added.

@mscanlan-git I think we probably want to create a permission specifically for the ability to invite hosts. You could do this by adding the permission to the Event model, e.g.:

class Event(models.Model):
    """
    Captures information on events such as datathons, workshops and classes.
    Used to allow event hosts to assist with credentialing.
    """
    title = models.CharField(max_length=128)
    description = SafeHTMLField(max_length=10000, blank=True, null=True)
    category = models.CharField(choices=EventCategory.choices, max_length=32)
    host = models.ForeignKey("user.User", on_delete=models.CASCADE)
    added_datetime = models.DateTimeField(auto_now_add=True)
    start_date = models.DateField(default=timezone.now)
    end_date = models.DateField(default=timezone.now)
    slug = models.SlugField(unique=True)
    allowed_domains = models.CharField(blank=True, null=True, validators=[
                                       validators.validate_domain_list], max_length=100)

    class Meta:
        unique_together = ('title', 'host')
        permissions = [('view_all_events', 'Can view all events in the console'),
                       ('invite_event_host', 'Can grant event host status to a user'),
                       ('view_event_menu', 'Can view event menu in the navbar')]

Some other notes:

  • After modifying the permission, you would need to generate a migration with python manage.py makemigrations and then apply the migration with python manage.py migrate.

I have added the migration file for this change, as well as add the decorator @permission_required('user.invite_event_host')

  • We would need to modify the "Invite event host" button to make it clear who can and cannot use it (e.g. deactivate it or hide it for people without the invite_event_host permission?

For this, I added {% if perms.self.invite_event_host %} in the template to make sure only users with the permission can view the button.

  • Currently the invite_event_host is displayed and active for users who are already event hosts. What is the desired behaviour in this situation?

This is the last bit I'm missing. I have tried numerous ways to enforce this, but the result seems to be binary each time (either the button is hidden for both event host and non-event host users, or vice versa). I committed the changes I made to add template tags, but I would like help as to how I can get this implemented conditionally.

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.

3 participants