PR #547 introduced a UI bug where the comment score tooltip would
show the wrong values (and in fact, the displayed score was also
wrong, though this was not originally noticed).
Specifically, a comment with e.g. +4 | -1 would display in the tooltip
as being +4 | -4 and display a score of 4. The desired behavior would
be +4 | -1 and score of 3. Precisely, the upvote value was displayed
for each of upvotes, downvotes, and net score.
Root cause was the `@lazy` decorator at:
`files.classes.comment._score_context_str(⋅)`
`@lazy` is very dumb. I don't entirely know why we don't just use
`functools.cache`, but we use `@lazy` everywhere. It is entirely
ignorant of the parameters to a function--not a substitute for
memoization.
comments.html contains the following snippet:
{%- set ups = c.upvotes_str(render_ctx) -%}
{%- set score = c.score_str(render_ctx) -%}
{%- set downs = c.downvotes_str(render_ctx) -%}
Each of those three functions internally calls to `_score_context_str`
but with different arguments. The first call to upvotes gets cached by
`@lazy` and the two subsequent calls get the upvotes string, rather than
what they wanted. It's a cheap enough operation that it's not really
worth memoizing, so we just remove the decorator.
changed in it
the comments.html template (along with submission.html) has numerous
undesirable properties which i will describe now. unless you are very
familiar with the codebase, it can be extremely difficult to grok.
this is pretty insane as there is nothing fundamentally complex about
the goal of comments.html: return a component that shows a username
and info, reports if any, comment content, and actions a user can
take.
this behemeoth was initially 886 lines in the old version of this
codebase, and this is with awards and a lot of other cruft removed.
anyway, the maintainability of this file is about on par with some
legacy application that keels over and dies if you sneeze vaguely
in its direction.
the nicest thing i can say about it is that it isn't currently
crashing.
anyway some of the problems include:
* large, splittable components, are not split into separate files.
this makes it incredibly difficult to find or make changes across
the template and makes it nearly impossible to find or change a
specific thing.
this is most easily exemplified in the modals, which should by all
accounts be separate templates, just inlined into comments.html.
* the nesting is oftentimes incorrect.
inexplicably, probably out of laziness from when the code was first
written, things will end up fully left aligned, while multiple layers
deep into a nesting context.
if an if statement or an endif is changed, it is *incredibly*
difficult to figure out where the error was. you can't trust the
nesting.
* multiple repeated checks for things that are always true.
this is probably a symptom of the above two problems but it's very
noticeable once you fix the nesting. for example there is a block
near the very top of the actions bar which checks for
parent_submission which effectively checks "is this in a post" (this
commit won't complain about parent_submission checks but i do have
opinions on those).
all of the action buttons further down the chain also check for
parent_submission, or even check inconsistently (by using if c.post)
within this context this is a completely unnecessary check in this
context.
while it is potentially useful (and in fact because #251 requires we
dismantle the assumption a little bit) to have these checks now, the
fact that they were initially added shows that when the code was all
initial written, there was little care into thinking about comment
state.
* mobile actions are duplicated and duplicated inline.
i actually do find it probably pretty hard to support this normally
given the codebase's DOM so whatever, duplicate the things, *but* if
we're going to do that, inlining it into the middle of an incredibly
long template is really difficult to comprehend as a design decision.
...anyway yeah this PR intends to fix these problems and enable work
to be done on #251. this is a "perfect is the enemy of good" commit.
it doesn't change much fundamental and is not intended to erase the
sins of the original file, but at least make it maintainable.
this also fixes a minor bug with #473 where the GIF modal was left
in by accident.
these functions i don't believe exist upstream and i guess were to
diagnose an issue with author_ids not being saved properly. anyway not
really useful now as if an author_id doesn't exist, some major db
corruption has prolly already happened.
* fix 401-302-401-302-429 loop
* don't logout users on bad form key, just treat the request as unauthenticated
* Handle None/empty case in validate_formkey.
A supplied empty formkey, or the lack of a supplied formkey (None) is not a valid formkey. Handle this inside the function rather than at the call-site.
* Validate as false if no hashstr or string
Co-authored-by: Snakes <104547575+TLSM@users.noreply.github.com>
Ported in from upstream with adjustments for TheMotte, most notably
universal default to 'new' and fixes to 'hot'. Lumped into this PR
because eager comment loading uses it.
Ported in logic from upstream to use SQLAlchemy eager loading instead
of repeated queries when building a submission_listing. Adjusted
loaded relationships to include only those used on TheMotte.
Using test data from seed_db, before and after:
GET /
|----------|--------|--------|--------|--------|--------|------------|
| Database | SELECT | INSERT | UPDATE | DELETE | Totals | Duplicates |
|----------|--------|--------|--------|--------|--------|------------|
| default | 83 | 0 | 0 | 0 | 83 | 72 |
|----------|--------|--------|--------|--------|--------|------------|
Total queries: 83 in 0.031s
GET /
|----------|--------|--------|--------|--------|--------|------------|
| Database | SELECT | INSERT | UPDATE | DELETE | Totals | Duplicates |
|----------|--------|--------|--------|--------|--------|------------|
| default | 14 | 0 | 0 | 0 | 14 | 0 |
|----------|--------|--------|--------|--------|--------|------------|
Total queries: 14 in 0.00718s
Removes the following awards / fields on User:
- flairlock
- progressivestack
- bird
- longpost (pizzashill)
- marseyawarded
- rehab
- deflector
- mute
- unmutable
- eye (All-Seeing Eye)
- alt (Alt-Seeing Eye)
Primarily motivated by starting to remove some un-Mottelike cruft
from core commenting/posting routes. Cleared out other inapplicable
awards while in the process.
