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

Fix/8694r3yfv community enhancement with privacy #85

Merged
merged 19 commits into from
Sep 25, 2024

Conversation

memorisanka
Copy link
Contributor

@memorisanka memorisanka commented Sep 16, 2024

https://app.clickup.com/t/8695gbbqh
https://app.clickup.com/t/8695gbc7n
https://app.clickup.com/t/8695gbgxm

Hi everyone!
I have added some bug fixes. Could you check it? :)

Summary by Sourcery

Implement a custom 403 permission denied view and enhance community privacy features by displaying access restrictions. Add validation and messaging for moderator management, and introduce a user-friendly 403 error page. Include tests to ensure correct handling of non-existing moderator removal.

New Features:

  • Introduce a custom 403 permission denied view to handle unauthorized access attempts.

Bug Fixes:

  • Fix the issue where users could attempt to remove non-existing moderators by adding validation and error messaging.

Enhancements:

  • Enhance the CommunityListView to display a lock icon for private communities that the user cannot access.
  • Add success messages for adding and removing moderators, and for successful community updates.

Documentation:

  • Add a new 403.html template to provide a user-friendly error page for forbidden access.

Tests:

  • Add a test case to verify the behavior when attempting to remove a non-existing moderator.

@memorisanka memorisanka requested a review from a team as a code owner September 16, 2024 18:03
Copy link
Contributor

sourcery-ai bot commented Sep 16, 2024

Reviewer's Guide by Sourcery

This pull request implements several enhancements to the community feature, focusing on privacy improvements and user experience. The changes include adding a custom 403 error page, improving community list view to show private communities, enhancing moderator management, and adding success messages for various actions.

File-Level Changes

Change Details Files
Implemented a custom 403 (Forbidden) error page
  • Created a new custom_permission_denied_view function
  • Added a new 403.html template
  • Configured the custom 403 handler in settings.py
core/views.py
core/templates/core/403.html
reddit/settings.py
Enhanced community list view to handle private communities
  • Added a has_access property to community objects in the context
  • Updated the community-list.html template to show a lock icon for private communities
core/views.py
core/templates/core/community-list.html
Improved moderator management in CommunityDetailView
  • Added success messages when adding or removing moderators
  • Implemented a check to prevent removing non-existent moderators
  • Added a new test case for removing non-existing moderators
core/views.py
core/tests/test_views.py
Added success messages for community updates
  • Implemented a success message in CommunityUpdateView when a community is updated successfully
core/views.py

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @memorisanka - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

core/views.py Show resolved Hide resolved
@@ -191,7 +206,13 @@
remove_moderator_form = RemoveModeratorForm(request.POST)
if remove_moderator_form.is_valid():
user = remove_moderator_form.cleaned_data["nickname"]
if not CommunityMember.objects.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (performance): Consider combining the moderator check with the remove_moderator operation.

The current implementation performs an additional database query to check if the user is a moderator before calling remove_moderator. Consider combining these operations to avoid unnecessary database queries, possibly by handling the case in the remove_moderator method itself.

            try:
                moderator = CommunityMember.objects.get(
                    community=self.object, user=user, role=CommunityMember.MODERATOR
                )
                self.object.remove_moderator(moderator)
            except CommunityMember.DoesNotExist:
                # Handle the case where the user is not a moderator
                pass

core/views.py Outdated Show resolved Hide resolved
@memorisanka
Copy link
Contributor Author

I have no clue why the custom 403 page is not working. Any ideas? :)

reddit/settings.py Outdated Show resolved Hide resolved
jacoor
jacoor previously approved these changes Sep 20, 2024
@jacoor
Copy link
Contributor

jacoor commented Sep 20, 2024

@memorisanka so the tests fails and there is good reason for it. Please update the 403 page to something generic, like "you don't have access to this content" or something generic. And update the failing test with it.
Thank you!

@memorisanka
Copy link
Contributor Author

The test is fixed :)

@memorisanka memorisanka merged commit 81de616 into dev Sep 25, 2024
2 checks passed
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