Skip to content

Conversation

Oaphi
Copy link
Member

@Oaphi Oaphi commented Sep 5, 2025

Fixes 2 server errors (likely no longer possible due to us switching to soft deletions of users, but we still have to account for full deletion requests and issues caused by legacy behavior of hard-deleting users) and a couple of other, non-crashing errors:

  1. 500 when trying to render a post deleted by a user that no longer exists;
  2. 500 when trying to render a post with reactions by a user that no longer exists;
  3. Unexpected request (always fails) for comment thread locking when opening the modal due to selector conflicts;
  4. Comment threads lacking the locked_at column despite being lockable (UnknownAttributeError error);
  5. Accessing thresholds for abilities with one or more thresholds set to 0 (FloatDomainError error);
  6. Lack of feedback for validation failures when trying to restrict comment threads (as a bonus, trying to archive, delete, follow, or lock a thread no longer reloads the full page, just updates the the thread in place);

I haven't checked if there are open issues because this is a followup for a chat discussion, but feel free to link them if you know of any.

Note for reviewers: this PR requires migrations to be run when checked out and rolled back when left to preserve your sanity. Don't forget to rails db:migrate -> rails db:rollback as needed.

@Oaphi Oaphi requested review from cellio and ArtOfCode- September 5, 2025 19:34
Copy link

codecov bot commented Sep 5, 2025

Codecov Report

❌ Patch coverage is 97.14286% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.73%. Comparing base (e6694c9) to head (027d186).
⚠️ Report is 60 commits behind head on develop.

Files with missing lines Patch % Lines
app/controllers/comments_controller.rb 95.91% 2 Missing ⚠️
Additional details and impacted files
Components Coverage Δ
controllers 71.47% <95.91%> (+0.20%) ⬆️
helpers 81.96% <ø> (ø)
jobs 60.00% <ø> (ø)
models 88.96% <100.00%> (+0.59%) ⬆️
tasks 61.11% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@cellio cellio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested posts and reactions from deleted users. (I don't have any destroyed ones, so we should double-check this on the dev server.) LGTM.

@Oaphi Oaphi changed the title Assorted fixes for server errors when rendering posts Assorted fixes for errors Sep 5, 2025
@Oaphi Oaphi requested a review from cellio September 6, 2025 05:20
@Oaphi Oaphi force-pushed the 0valt/general-fixes branch from be37144 to a191df6 Compare September 7, 2025 04:41
Copy link
Member

@cellio cellio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I recommend a second code-reviewer as a sanity check, particularly if you're going to handle those TODOs about new routes here rather than later.

redirect_to comment_thread_path(@comment_thread.id)
end

def archive_thread
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other reviewers: the code in this block is copied from elsewhere, no changes to logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more precise, these mehtods are more or less extractions of individual cases of thread_restrict and thread_unrestrict actions. It's intended that they'll become proper actions in the future (cc @trichoplax given that you are working on thread followers - there'll now be follow_thread and unfollow_thread methods)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a note for other reviewers - I went ahead and split the thread_restrict action, so archiving, deleting, locking, and following threads need to be tested.

Copy link
Member

@cellio cellio Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested archive/restore, delete/undelete, lock/unlock, and follow/unfollow, all from the in-page tools menu. All of that worked fine.

I then repeated the tests on the thread page. For lock, archive, delete, and follow, the action worked but when the page was refreshed, it showed the "collapse" and "show thread" controls (from the in-page view), which don't make sense here:

screenshot with extra controls highlighted

Reloading the page removes them, so this is only an issue on the immediate response to the action, which I guess is assuming context from the in-page view? (I.e. the header with the Tools menu etc can't tell where it is, maybe?) Not a big deal but a bit weird -- can be deferred if this isn't a trivial fix.

Interestingly, this does not happen for unlock, restore, undelete, and unfollow. I looked for a difference in the controller (since these used to be rolled up into two methods, "restrict" and "unrestrict"), but I'm not spotting it if that's where it's coming from. The comment_thread view isn't changed on this PR, but this behavior doesn't happen on the dev server as of 5c83ab90 (2025-09-05 19:17:57Z). That probably means it's in the Javascript, heaven help us.

Copy link
Member Author

@Oaphi Oaphi Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, that's likely just a side-effect of that we now replace the thread's content instead of reloading the whole page. It works as expected (this obviously doesn't happen for thread_unrestrict actions as those retain the old behavior for now unless I end up splitting those too), I suspect we also have the same issue when expanding threads from inline view. Good catch, let's fix it while we are at it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now, @cellio

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working now, thanks!

@Oaphi Oaphi requested a review from cellio September 9, 2025 00:29
Copy link
Member

@cellio cellio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruby code LGTM. JS code doesn't raise any alarms but we should have someone who understands JS better take a look at that part.

I tested the items listed in the PR description and everything that I'm able to test works fine, except for one minor weird rendering thing described in an earlier comment. It's ok with me if we file an issue to fix that later. (fixed) Also, as mentioned earlier, I don't have any destroyed users to experiment on; I don't see anything here that would make that worse, and it very likely fixes problems from that, so if we can't test it in dev, that shouldn't block us. I think we have some destroyed users on the dev server but am not certain.

@Oaphi Oaphi merged commit 7b952d7 into develop Sep 9, 2025
13 checks passed
@Oaphi Oaphi deleted the 0valt/general-fixes branch September 9, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants