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.

Sign up for GitHub

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

Jump to bottom

feat: implement club announcement restrictions #1733

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

feat: implement club announcement restrictions #1733

wants to merge 1 commit into from

Conversation

JasonGrace2282
Copy link
Member

@JasonGrace2282 JasonGrace2282 commented Oct 23, 2024 •
edited
Loading

This makes it so that announcements that are created by a restricted club are only shown to people who are allowed to access the restricted club.

@coveralls
Copy link

coveralls commented Oct 23, 2024 •
edited
Loading

Coverage Status

coverage: 79.155% (+0.06%) from 79.098%
when pulling 56c379c on JasonGrace2282:club-ann-restrict
into f80a619 on tjcsl:dev.

@alanzhu0
Copy link
Member

So it seems fine performance-wise. But I think you need to be more careful and check everywhere this query is used. I'm not sure that this one instance will cover everything. Basically, shore it up some more.

  • Can you also make it so people not on the allowed list/groups can't subscribe to the activity?
  • And when those groups are updated, maybe you can consider also automatically removing the people removed from those groups from the subscribed list
  • Also, we discussed this, but maybe instead of checking if the activity is restricted, just have a checkmark to specifically restrict club announcements. So restricted activities could still make their announcements public. I don't think we need the case where a public activity has restricted announcements. This may also have performance benefits.
  • And for that, can you add some text on the post announcements page and the activity edit page that states that the announcements are restricted?

alanzhu0
alanzhu0 requested changes Jan 24, 2025
View reviewed changes
Copy link
Member

@alanzhu0 alanzhu0 left a comment

Choose a reason for hiding this comment

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

see above

@JasonGrace2282 JasonGrace2282 force-pushed the club-ann-restrict branch 2 times, most recently from 41712e0 to ebd1345 Compare February 25, 2025 19:04
@JasonGrace2282 JasonGrace2282 marked this pull request as ready for review February 25, 2025 19:04
@JasonGrace2282 JasonGrace2282 requested a review from a team as a code owner February 25, 2025 19:04
alanzhu0
alanzhu0 requested changes Mar 15, 2025
View reviewed changes
Copy link
Member

@alanzhu0 alanzhu0 left a comment

Choose a reason for hiding this comment

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

Thanks Aarush, great work and almost done, just a few small changes

intranet/apps/eighth/models.py Outdated
Returns:
Whether the user can subscribe to the activity.
"""
return user.is_superuser or (
Copy link
Member

Choose a reason for hiding this comment

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

for consistency can you make this user.is_eighth_admin?

intranet/apps/eighth/models.py Outdated
return user.is_superuser or (
self.subscriptions_enabled
and user.is_authenticated
and (not self.restricted or (user in self.users_allowed.all() or user.groups.filter(id__in=self.groups_allowed.all()).exists()))
Copy link
Member

Choose a reason for hiding this comment

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

user in self.users_allowed.all() can also be replaced with a .filter(...).exists()

intranet/apps/announcements/models.py Outdated
@@ -111,6 +124,10 @@ class Announcement(models.Model):
added = models.DateTimeField(auto_now_add=True)
updated = models.DateTimeField(auto_now=True)
groups = models.ManyToManyField(DjangoGroup, blank=True)
public = models.BooleanField(
default=False,
help_text="Whether to show the announcement, even if the club is restricted (club announcements only)",
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 this should be more clear. it should be to show the announcement publicly to all users. right now it sounds like nobody can see the announcement if it is restricted...

Copy link
Member

Choose a reason for hiding this comment

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

also it would be good if you can make it clearer that restricted activities have announcements restricted to members of the allowed list by default

@JasonGrace2282 JasonGrace2282 mentioned this pull request Mar 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanzhu0 alanzhu0 alanzhu0 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JasonGrace2282 @coveralls @alanzhu0