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

RES-1910 Event chat and attendee count inconsistencies #668

Merged
merged 10 commits into from
Aug 23, 2023

Conversation

edwh
Copy link
Collaborator

@edwh edwh commented Jul 10, 2023

Based on RES-1855 so the diffs are confusing until that is merged.

  • Move the maintenance of the volunteer count and the Discourse thread into an observer, which is triggered by changes to EventsUsers. This means we don't have to rely on the calling code to remember to do it...except for deletes (see this).
  • Background the actual processing this via events and listeners in the usual way
  • Introduce a method into DiscourseService to remove a user from the private message thread.
  • When the host of a group is added to an event, they should become a host of the event.
  • Allow fetching of an event over the API if it is not yet approved. No reason not to do this, and it helps with testing.

Copy link
Contributor

@ngm ngm left a comment

Choose a reason for hiding this comment

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

Couple of questions about the logic of who is set as a host of an event when joining the event.

$u = User::find($user);

// A host of a group who is added to an event becomes a host of the event.
$eventRole = $u && $u->role == Role::RESTARTER ? Role::RESTARTER : Role::HOST;
Copy link
Contributor

Choose a reason for hiding this comment

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

This would make network coordinators and admins hosts of the event, too, right?

I can think of cases where that's not going to match reality (e.g. me, going to an event). But, probably generally fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the case where a host of one group attends an event by another group, this would automatically make them host of that event? I don't think that should be the case. It should only turn hosts of the event's group into hosts of the event, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The second point would also fix the first. This logic isn't well-tested in any case, so I've added a test for it to check that people are/aren't added as hosts appropriately.

@edwh edwh changed the title WIP RES-1910 event chat counts RES-1910 event chat counts Aug 7, 2023
Copy link
Contributor

@ngm ngm left a comment

Choose a reason for hiding this comment

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

I think the logic for setting event role needs changing, see comment.

$u = User::find($user);

// A host of the group who is added to an event becomes a host of the event.
$eventRole = $u && $u->role == Role::HOST && Fixometer::userIsHostOfGroup($party->group, $user) ? Role::HOST : Role::RESTARTER;
Copy link
Contributor

@ngm ngm Aug 22, 2023

Choose a reason for hiding this comment

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

I think this should just be

$eventRole = $u && Fixometer::userIsHostOfGroup($party->group, $user) ? Role::HOST : Role::RESTARTER;

as there could be admins or network coordinators who are host of the group, and should get set with $eventRole = Role::HOST, even if their overall role ($u->role) might not be Role::HOST.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I've made that change.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 22 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@edwh edwh merged commit 6368fde into develop Aug 23, 2023
@edwh edwh changed the title RES-1910 event chat counts RES-1910 Event chat and attendee count inconsistencies Aug 23, 2023
@ngm ngm deleted the RES-1910_event_chat_counts branch June 19, 2024 16:21
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.

2 participants