Skip to content

Conversation

@Oaphi
Copy link
Member

@Oaphi Oaphi commented Oct 26, 2025

This PR includes assorted improvements in preparation for more significant changes:

  • do_soft_delete method on the User model is renamed to soft_delete to match the naming convention (do_* methods must have a counterpart without do_);
  • soft deletion tests are expanded to cover both profile and user deletions;
  • CommunityUser now has a corresponding soft_delete method;
  • HTML markup of the deletion confirmation email is fixed (we were missing a closing </li> tag);
  • two new global site settings are added: NetworkURL and NetworkName & as many views as feasible have been switched to them from the hardcoded https://codidact.com/ and Codidact strings (related: Reference to the Codidactyl in non-Codidact instances #1396, Use network name in email subjects without baking in "Codidact" #1664);
  • a new User.system static method is added to simplify system user lookup;
  • tests no longer attempt to make requests to external URIs (system tests are still allowed to do so as they need the assets to work) thanks to the WebMock gem (test env only);
  • 2FA mailer actions are now fully covered with tests;
  • a fix for the regular expression checking that the post contains image links with no alt text (see QPixel warns me that images in a post lack alternative captions, yet no images exist. #1889 - not closing it as we can benefit from further improvements to validation logic there, but at least the main concern is addressed by the PR);
  • comment pings now properly register and render for system users (re: meta:294917):
    2025-11-05_03-44

To be able to test the changes, it is necessary to:

  • run bundle install to add the new gem (tests will break without it);
  • run rails db:seed to populate the new site setting;

A request for reviewers: we need to thoroughly check that the mailers actually pick up the setting, especially the donation ones. I checked some, but not all yet.

@codecov
Copy link

codecov bot commented Oct 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.38%. Comparing base (74022de) to head (6e6d1a3).

Additional details and impacted files
Components Coverage Δ
controllers 74.38% <100.00%> (+0.15%) ⬆️
helpers 84.87% <100.00%> (+0.19%) ⬆️
jobs 79.24% <100.00%> (+13.86%) ⬆️
models 89.92% <100.00%> (+0.25%) ⬆️
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.

@Oaphi Oaphi force-pushed the ovalt/user-restore branch 2 times, most recently from 5a22e92 to 0b1c610 Compare October 27, 2025 03:41
@Oaphi Oaphi force-pushed the ovalt/user-restore branch from 0b1c610 to 1788af1 Compare October 27, 2025 04:07
@Oaphi Oaphi marked this pull request as ready for review October 27, 2025 04:16
@Oaphi Oaphi requested review from cellio and trichoplax October 27, 2025 04:16
@Oaphi Oaphi requested a review from ArtOfCode- October 27, 2025 04:16
@cellio

This comment was marked as resolved.

@cellio
Copy link
Member

cellio commented Oct 28, 2025

Testing notes (will edit this comment as I go):

  • NetworkName in email?

    • new-user signup: confirmed
    • self-deleted account: confirmed
    • password change: does not use a network name, but does have a hard-coded URL for resets that was already present (not a regression). I initially thought we could use the new NetworkURL setting instead, but the URL in the email is to a particular community, so that won't work.
    • email to all moderators: confirmed has network name in subject line
    • email to all users: did not previously include network name and doesn't now either (someday we might make that consistent with the mod email, but can wait) -- see details below
    • subscription email: ? I haven't gotten my dev server to send subscription email at all, so I can't check the content, but this looks like all the other email changes so don't block on this.
    • not sure how to test donation email without setting up actual Stripe stuff in the dev env? (I don't have an API key)
    • legal deletion: confirmed
  • Account deletion:

    • self-delete and mod delete work as expected
    • haven't checked yet if this is a regression, but if a mod deletes a profile on one community, the "all communities" list for the user still shows the link to everyone (mods see the deleted user, others see the skull & crossbones) -- I think this is what we did already but noting it here to confirm - not a regression (this is what we do already)
  • sanity check on system user change: renamed a comment thread and the comment was correctly attributed to System (this is an existing use of that code that I knew about)

--

Regarding the admin email to mods/all users, this is how the subject lines and senders look now. Mod mail includes the network name; all-user mail doesn't. We can update it here or spin it off -- don't care which, just noticed in passing.

screenshot

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.

I got through the code and seeds but haven't looked at tests yet. I'll come back for those, but meanwhile, here are some assorted small comments.

@@ -1,9 +1,13 @@
<h1>Keyboard tools</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Haven't confirmed visually because I don't yet know how to navigate to this page...

Copy link
Member Author

@Oaphi Oaphi Oct 28, 2025

Choose a reason for hiding this comment

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

Apparently, the route got explicitly removed (while the view and the controller action are still there) in 9815057#diff-959bc9abc46a55332bb64d5155a79323afa75a50ec1a2137ddd22d926f62c6c5L215. @ArtOfCode- (just making sure we are all up to speed regarding the removal)?

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 (someone else should also review). Testing notes/status are in my earlier comment. I read through the changes in the test cases and they look like they're testing the right things to me (did not run). I think my remaining earlier comments are either N/A or deferrable -- nothing blocking there. (Do note that I can't currently test donations.)

@Oaphi Oaphi requested a review from cellio November 5, 2025 00:56
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.

Confirmed this fixes https://meta.codidact.com/posts/294917. (Someone other than me should still review the code, particularly JS.)

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.

4 participants