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

Only return published blog entries. #1757

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Only return published blog entries. #1757

wants to merge 2 commits into from

Conversation

marksweb
Copy link
Contributor

This fixes #1755 to only return published blog entries, even for staff users.

Unpublished entries can still be previewed via the admin.

@marksweb
Copy link
Contributor Author

marksweb commented Nov 20, 2024 •
edited
Loading

On a side note, my test suite is failing me and I'm not sure why.

======================================================================
ERROR: blog.tests (unittest.loader._FailedTest.blog.tests)
----------------------------------------------------------------------
ImportError: Failed to import test module: blog.tests
Traceback (most recent call last):
  File "/Users/mark/.local/share/uv/python/cpython-3.12.7-macos-aarch64-none/lib/python3.12/unittest/loader.py", line 396, in _find_test_path
    module = self._get_module_from_name(name)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mark/.local/share/uv/python/cpython-3.12.7-macos-aarch64-none/lib/python3.12/unittest/loader.py", line 339, in _get_module_from_name
    __import__(name)
  File "/Users/mark/dev/djangoproject.com/blog/tests.py", line 2, in <module>
    from test.support import captured_stderr
ModuleNotFoundError: No module named 'test'

Discovered the reason for this; #1758

@marksweb marksweb requested a review from a team November 20, 2024 23:02
@marksweb marksweb mentioned this pull request Nov 20, 2024
Closed
pauloxnet
pauloxnet reviewed Nov 21, 2024
View reviewed changes
blog/views.py Outdated Show resolved Hide resolved
bmispelon
bmispelon requested changes Nov 21, 2024
View reviewed changes
Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

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

Thanks for the quick response on this, nice work 👍🏻

If you allow me to change/extend the scope of this PR a little bit, I think it's not enough to test for user.is_staff but we should also make sure that the user has write permissions on the BlogEntry model (since this feature is meant to be used for previewing).

And there should also be some tests added (I'm suprised there weren't already, or that if there were they were still passing after this change):

  • anon users can't see unpublished entries at all (list or detail)
  • logged in users (non-staff) can't see unpublished entries at all
  • staff users without write permission on BlogEntry can't see unpublished entries at all
  • staff users with write permission on BlogEntry can't see unpublished entries in the list, but can view the detail page

@jefftriplett
Copy link
Member

@marksweb this is great. Thank you for jumping in on this.

@marksweb
Copy link
Contributor Author

And there should also be some tests added (I'm suprised there weren't already

Yes I was surprised & expecting you to suggest this 😄

@marksweb marksweb self-assigned this Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pauloxnet pauloxnet pauloxnet left review comments

@jefftriplett jefftriplett jefftriplett left review comments

@bmispelon bmispelon bmispelon requested changes

Assignees

@marksweb marksweb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unpublished Blog Entries show up for staff in list pages
4 participants
@marksweb @jefftriplett @bmispelon @pauloxnet