comment moderation fix item value check

[fliiiix]

The item check should happen before we try to access
the value not after otherwise it results in a backtracke like this:

gunicorn[441]: 2022-02-17 20:46:52,526 ERROR: GET /id/2038/delete/XXXXXXXXXXXX
gunicorn[441]: Traceback (most recent call last):
gunicorn[441]: File "/opt/isso/lib/python3.6/site-packages/isso/init.py", line 150, in dispatch
gunicorn[441]: response = handler(request.environ, request, **values)
gunicorn[441]: File "/opt/isso/lib/python3.6/site-packages/isso/views/comments.py", line 635, in moderate
gunicorn[441]: thread = self.threads.get(item['tid'])
gunicorn[441]: TypeError: 'NoneType' object is not subscriptable

Now we return a item NotFound instead.

Signed-off-by: fliiiix hi@l33t.name

[ix5]

Nice find! And already well documented, good job.

Would you mind changing the commit message title to make it more clear which parts are affected and how? Something like:

views: comments: moderation: Check value earlier

[fliiiix]

sure how about this?

[ix5]

Thanks for your contribution! I added some tests for the moderation feature in https://github.com/ix5/isso/tree/moderating-add-testing, would you mind expanding them a bit?

Also, the correct response to the moderate API call probably shouldn't be a literal Yo, maybe you could change that as well? (Don't forget to change API docs).

[fliiiix]

sure i hope i find some time to think about some more cases and will create a MR when ready https://github.com/fliiiix/isso/tree/moderating-add-testing

[fliiiix]

One question i had while working on the tests is would you be open to a refactoring the tests into smaller tests so instead of having TestModeratedComments with multiple test cases in it to have a tests for each test case?

[ix5]

Thank you for tackling the test deficiencies! I already like what you're doing on your branch a lot.

One question i had while working on the tests is would you be open to a refactoring the tests into smaller tests so instead of having TestModeratedComments with multiple test cases in it to have a tests for each test case?

Hmm, I myself am not too involved in testing conventions, but there's two issues I can think of.
First, splitting the tests into smaller parts might mean they take longer to run (i.e. no longer one setup/teardown ("hoist"?) for multiple assertions). Maybe you could provide some before/after data for only test_comments.py, for now?
Second, mixing small and large test blocks might make it harder for newcomers to this project to decipher from the existing code how they should write their tests - one large block of asserts or one block per assert?

So a refactor ideally should include all tests and also make it clear in the docs what testing structure this project expects.

For now, I think refactoring only test_comments.py is OK, anything else I'll leave @jelmer to weigh in first.

[fliiiix]

Ok i have a look what i can do. From my quick assessment i didn't see something which would take very long to construct for the tests. So while they will run a bit slower overall they should be more clear to read and understand and we reduce the side-efects we have from the things that run before and will influence our test.

But I guess i will create then an extra PR where i change that in test_comments.py so we have something to discus.

[ix5]

(For anyone coming across this in the future, the testing discussed here was PR'd in #805)