search

numist / this-might-be-offensive

the stuff that makes tmbo tick
https://thismight.be/offensive/
29 stars 12 forks source link

Fix numAllowedUploads(): sum up votes #92

Closed leonplanken closed 9 years ago

leonplanken commented 9 years ago

Previously, numAllowedUploads() iterated over all uploads of the past 6 months but seemed to only use the votes on the last one.

jaswope commented 9 years ago

Good catch

plytro commented 9 years ago

This wasn't needed.

I'll recap this: https://thismight.be/offensive/?c=comments&fileid=393243#284516937

The group by causes you to only have two results rows from the query.

count(vote) | vote
------------|-----------------
1000        | this is good
400         | this is bad

No summation will actually be happening because you only have one record for each vote type.

leonplanken commented 9 years ago

I'll just leave this here... https://thismight.be/offensive/?c=comments&fileid=266492#284516967

I think this change might make the code just a tad clearer but of course the commit message is off. Sorry for the inconvenience. But it was fun creating my first pull request (and looking at / 'contributing' to tmbo code).

jaswope commented 9 years ago

That will teach me to merge stuff while mobile. I'll have to fix it when I can.

numist commented 9 years ago

This new code is not wrong; we can leave it in.

The SQL query guarantees there can only be one row for each vote type in the switch statement and if some day we manage to screw that up, this code will degrade more gracefully/accurately than before.

leonplanken commented 9 years ago

It's just the commit message that's off. :-) So that could possibly be amended.

numist commented 9 years ago

shrug There are a lot of commit messages in this codebase that say that my code is wrong :)