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

Events: show inactive participants to host and Admin #1923

Merged

Conversation

superryeti
Copy link
Contributor

Context:
Currently on Events Homepage, and on Event management page(admin console), we can only see the list of people approved to the event. We cant see who has been rejected to the event or if someone has withdrawn access to the event.
It will be helpful to hosts if they can have access to the unsuccessful participants and the reason that they were rejected.

This PR adds another table on the existing modal which lets us switch tabs between active, inactive participants(the wording is a bit wrong here, maybe this should be inactive applicants/unsuccessful aplications)

Pictures for reference

Active tab

InActive Tab

@superryeti
Copy link
Contributor Author

Tab variation option(this one doesnot have the dark background and black text)
image

@tompollard
Copy link
Member

Listing application history for users is a good idea, but the changes in this PR need a little more thought. Possibly just a case of tweaking the language (e.g. "Inactive Participant" is an odd name for someone who is not a participant; "inactive applications" is an odd way to describe an application that was withdrawn or closed).

@superryeti
Copy link
Contributor Author

superryeti commented Mar 9, 2023

Listing application history for users is a good idea, but the changes in this PR need a little more thought. Possibly just a case of tweaking the language (e.g. "Inactive Participant" is an odd name for someone who is not a participant; "inactive applications" is an odd way to describe an application that was withdrawn or closed).

Could we create 1 tab per type, for example

  1. Approved
  2. Not Approved
  3. Withdrawn
  4. Revoked

@superryeti
Copy link
Contributor Author

Listing application history for users is a good idea, but the changes in this PR need a little more thought. Possibly just a case of tweaking the language (e.g. "Inactive Participant" is an odd name for someone who is not a participant; "inactive applications" is an odd way to describe an application that was withdrawn or closed).

Could we create 1 tab per type, for example

  1. Approved
  2. Not Approved
  3. Withdrawn
  4. Revoked

Let me think a bit about the names and get back. The multiple tab option is also not looking very good

@superryeti
Copy link
Contributor Author

Hello @tompollard, thank you for your feedback. I agree that the language in the menu name and the application status needs to be more precise and accurate. I have the following suggestions:

  1. Use Application History instead of Inactive Application as the menu name and then include the status on a column
  2. Or use different tabs for different types of applications, such as:
    a. Not Approved Applications
    b. Withdrawn Applications
    c. Revoked Applications
    d. Closed Applications

2nd option would allow Host to filter applications by status

What do you think of these suggestions? Do you approve them or do you have any other feedback? Please let me know.

@superryeti superryeti force-pushed the au/event/feature/show_inactive_participants branch from dc503a0 to 01c09a2 Compare March 15, 2023 15:42
@kshalot
Copy link
Member

kshalot commented Jul 31, 2023

Hey, @tompollard! (@alistairewj I'm also roping you in. In case you have any comments)



I’m doing a small review of the old events PRs to see what can be pushed forward and what needs details. I resolved the conflicts in this branch so we can see the changes.

Amit proposed two ways (#1923 (comment)) to get rid of the “inactive” wording before he left.

He mentioned that the multiple tab option is not ideal, but functionally it’s closer to other existing flows (for example the training view, pictured below) and separates the different statuses in a cleaner way (it’s going to be way easier to answer more specific questions like “How many people have withdrawn from the event? Who was it?” without introducing some filtering capabilities).

Trainings

Showing those tabs in the “total participants” view might be confusing though, since they are not counted towards the total (the UI is visible in a screenshot below). There’s also no indication that anything apart from accepted participants will be visible when clicking the button. This could be fixed with a simple rewording (total participants -> accepted participants, view participants -> view applications) although it still wouldn't be ideal:

ViewApplications

We can also try making it granular and show it in the event details directly:

Granular

Similar changes would be made to the Events page (outside of the admin panel).

I think just keeping it simple for now and having a single button would be the better option because I feel that the UI is bad either way. Showing the participants in a modal feels more like trying to simplify the implementation of the events pages than a conscious design decision, but I feel that if we want to rework the page, it should be a separate issue/PR. I'm curious to hear your thoughts.

physionet-django/events/views.py Outdated Show resolved Hide resolved
physionet-django/events/views.py Outdated Show resolved Hide resolved
physionet-django/events/views.py Outdated Show resolved Hide resolved
physionet-django/console/views.py Outdated Show resolved Hide resolved
physionet-django/console/views.py Outdated Show resolved Hide resolved
@Rutvikrj26
Copy link
Contributor

@kshalot Thanks for the comments. I hadn't considered the efficiency and number of queries.
I've updated the codebase and resolved the conflicts with the dev.
Please check and let me know if this looks good.

@Rutvikrj26 Rutvikrj26 force-pushed the au/event/feature/show_inactive_participants branch from 2d1a44b to 1785ad8 Compare November 1, 2023 21:14
@Rutvikrj26 Rutvikrj26 force-pushed the au/event/feature/show_inactive_participants branch from 1785ad8 to 70414fb Compare November 6, 2023 20:59
@Rutvikrj26 Rutvikrj26 force-pushed the au/event/feature/show_inactive_participants branch from 70414fb to 66a6689 Compare November 7, 2023 13:28
physionet-django/console/views.py Outdated Show resolved Hide resolved
physionet-django/console/views.py Outdated Show resolved Hide resolved
physionet-django/events/views.py Outdated Show resolved Hide resolved
physionet-django/events/views.py Outdated Show resolved Hide resolved
physionet-django/events/views.py Outdated Show resolved Hide resolved
physionet-django/events/views.py Outdated Show resolved Hide resolved
physionet-django/events/views.py Outdated Show resolved Hide resolved
@Rutvikrj26
Copy link
Contributor

@tompollard @kshalot requesting further review and/or merge.
PR #1925 partially builds on top of this PR, and would like to get this one complete & merged.
Thanks!

@kshalot kshalot self-requested a review November 8, 2023 19:56
Copy link
Member

@kshalot kshalot left a comment

Choose a reason for hiding this comment

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

A few tiny thoughts.

physionet-django/console/views.py Outdated Show resolved Hide resolved
physionet-django/console/views.py Outdated Show resolved Hide resolved
physionet-django/events/views.py Outdated Show resolved Hide resolved
@tompollard
Copy link
Member

Looks good, thanks!

@tompollard tompollard merged commit bdfa393 into MIT-LCP:dev Nov 16, 2023
8 checks passed
@Rutvikrj26 Rutvikrj26 deleted the au/event/feature/show_inactive_participants branch November 26, 2023 23:15
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.

4 participants