-
Notifications
You must be signed in to change notification settings - Fork 332
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
[Django Upgrade] [Reference PR] The rest of the fixes #10048
Changes from 1 commit
7c3b9ad
4a8bd61
bf7eb21
15f15e9
5746a84
c2c5e4e
f22e531
250b832
8052ea4
65b344b
205e560
1675f6f
391af60
c9c7f44
721ef90
75ffbb0
285d9a6
06f6fe9
724f59a
afa62a7
d15c88f
3f162f1
f971149
f41eb8a
dab1bbc
994fcfd
a54bbbe
699fc1c
0dc9220
cfb012e
cb25d71
720b6ec
6b7ce7f
0192907
51f4f48
41d4b50
65f39c8
0ad3f7c
a2dc7ba
2a0aa6e
66edda0
37bd4e4
bcd2c6e
88c057e
0ce6496
fc0d032
2fbc057
eb93348
07ee7e6
9e9c487
4e19413
d10824f
4e56c87
68acb17
57e81a6
0bc3d5c
7d3c644
51a6b1b
e21b460
1a70f61
f30f1ce
fca9e81
dffad34
db10898
0c1cc03
494e396
6548110
0bb5fc5
d69128c
7f6219b
3f97cd6
ec4da2f
6f64b38
6dcb706
b1845e4
95c9c83
ba02936
fd42db4
dd377cc
5f7fd3b
b57c74e
93081d4
b58c17f
5a24b8f
3f6a665
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,8 +103,8 @@ def test_get_object(self, req, preprint, plain_view): | |
def test_no_user_permissions_raises_error(self, user, preprint, plain_view): | ||
request = RequestFactory().get(reverse('preprints:preprint', kwargs={'guid': preprint._id})) | ||
request.user = user | ||
resp = plain_view.as_view()(request, guid=preprint._id) | ||
assert resp._headers['location'][1] == f'/accounts/login/?next=/preprints/{preprint._id}/' | ||
with pytest.raises(PermissionDenied): | ||
plain_view.as_view()(request, guid=preprint._id) | ||
|
||
def test_get_flagged_spam(self, superuser, preprint, ham_preprint, spam_preprint, flagged_preprint): | ||
request = RequestFactory().get(reverse('preprints:flagged-spam')) | ||
|
@@ -183,8 +183,8 @@ def test_correct_view_permissions(self, user, preprint, plain_view): | |
request = RequestFactory().get(reverse('preprints:preprint', kwargs={'guid': preprint._id})) | ||
request.user = user | ||
|
||
response = plain_view.as_view()(request, guid=preprint._id) | ||
assert response.status_code == 302 | ||
with pytest.raises(PermissionDenied): | ||
plain_view.as_view()(request, guid=preprint._id) | ||
Comment on lines
-186
to
+187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, this looks like something we need to fix on the permission side instead of the test itself There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be caused by DRF, where permission check now require all permissions instead of any >> view_permission = Permission.objects.get(codename='view_preprint')
>> user.user_permissions.add(view_permission)
>> user.save()
>> permission_required = ('osf.view_preprint',)
>> user.has_perms(permission_required)
>> True
>> permission_required = ('osf.view_preprint', 'osf.change_preprint',)
>> user.has_perms(permission_required)
>> False This is why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Final thoughts: I agree with @john Tordoff that this is more on the test than the code. The behavior of |
||
|
||
def test_change_preprint_provider(self, user, preprint, plain_view): | ||
change_permission = Permission.objects.get(codename='change_preprint') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests failed with:
Due to DRF changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a difference: the old test redirects to login while the new one raises exception? (though I feel the new one is the correct behavior but please double check.). We can also test this one on staging3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, to be clear the permission error prevents the test from completing, so it doesn't get to the redirect. I think it should redirect normally.