make sure comment and poll can only be added with egreement and… #1130
Conversation
Coverage reportTotal coverage
Report generated by 🧪jest coverage report action from 3838ba8 |
fuzzylogic2000
force-pushed
the
kl-2022-06-checkbox-tests
branch
5 times, most recently
from
1fb8103 to
ca15c1c
Compare
fuzzylogic2000
force-pushed
the
kl-2022-06-checkbox-tests
branch
2 times, most recently
from
ece164c to
2c7b968
Compare
fuzzylogic2000
changed the title
| @@ -93,6 +113,10 @@ def vote(self, request, pk): | |||
| self.request.data['agreed_terms_of_use'] | |||
| } | |||
| ) | |||
| else: | |||
| raise ValidationError({ | |||
There was a problem hiding this comment.
For the poll, it does not matter which error, I use, it always gives "Your answer could not be saved. Please check the data you entered again.". I will add an issue for that, but would not like to handle that in this PR.
There was a problem hiding this comment.
I think that comes from frontend, we catch errors there, but then always show 'Your answer...'. So we should probably show the actual message of error there?
There was a problem hiding this comment.
Let's comment in here: #1141
fuzzylogic2000
force-pushed
the
kl-2022-06-checkbox-tests
branch
3 times, most recently
from
c5172aa to
a11fb9b
Compare
|
@fuzzylogic2000 but the tests are working now? |
@goapunk Yes! I had to patch the reverse that is imported into the api. Which does make sense, but took me a while... |
| if hasattr(request, 'user'): | ||
| user = request.user | ||
| if user.is_authenticated: | ||
| user_has_agreed = \ | ||
| user.has_agreed_on_org_terms(organisation) | ||
| user_has_agreed = self._user_has_agreed(user) |
| if hasattr(request, 'user'): | ||
| user = request.user | ||
| if user.is_authenticated: | ||
| user_has_agreed = \ | ||
| user.has_agreed_on_org_terms(organisation) |
There was a problem hiding this comment.
Maybe we could still use user.has_agreed_on_org_terms here? Because we already have the organisation object here and then dont have to get it again in self._user_has_agreed.
There was a problem hiding this comment.
Weeeeeell, I changed it because otherwise the tests do not work. Which is a weird reason, but I just could not find anything faking a property. And I would still like to test in a4, so we cannot really use the property that we only have in a+. So, don't know. 😢
There was a problem hiding this comment.
Ah ok, I see! But doing all these queries to make the test work also doesnt seem right, no? Maybe we can just move the failing tests to Aplus? Or is it all of them?
There was a problem hiding this comment.
Yeah, for the poll it would be all of them. And I think, if we use user.has_agreed_on_org_terms in the API, we should also check that it is implemented and otherwise throw an error. Just relying on that it's there in the project also does not seem like the best way to go.
fuzzylogic2000
force-pushed
the
kl-2022-06-checkbox-tests
branch
from
a11fb9b to
3838ba8
Compare
|
@Rineee I changed the names of the tests and added a few doctsrings. I hope it is a bit more structured and easy to see what I actually test now. :) |
… add tests
I know why the test fails when run in the whole suite:
https://stackoverflow.com/questions/65292022/why-does-mock-patch-only-work-when-running-specific-test-and-not-whole-test-suit
But I couldn't fix it. :(
https://stackoverflow.com/questions/7293653/mocking-before-importing-a-module
https://stackoverflow.com/questions/1254370/reimport-a-module-in-python-while-interactive/1254379#1254379