General fixes & improvements #1976
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Tested:
|
…actually points to globalThis
…rror notification
…the magic number when checking upload sizes
…load_size (based on MaxRequestBodySize)
| def cleanup_site_settings | ||
| to_remove = [] | ||
|
|
||
| $site_settings_map.each do |community_id, names| | ||
| stale_settings = SiteSetting.unscoped | ||
| .where(community_id: community_id) | ||
| .where.not(name: names) | ||
| to_remove.push(*stale_settings) | ||
| end | ||
|
|
||
| return unless to_remove.any? | ||
|
|
||
| ActiveRecord::Base.transaction do | ||
| to_remove.each(&:destroy!) | ||
| end | ||
|
|
||
| puts "#{SiteSetting.model_name}: removed #{to_remove.size}" | ||
| end |
There was a problem hiding this comment.
After this PR is merged, we'll start automatically removing SiteSettings that are no longer present in seeds. My hope is that it'll evolve into a proper cleanup step in the future and then into a full solution for dynamically changing seeds.
There was a problem hiding this comment.
Question: if a setting is no longer in seeds but is in the DB, then code that still checks for that setting would continue to work in existing deployments and fail in new ones. How do we make sure that there's no referring code before removing, and especially auto-removing, a setting? I love having cleanup but do want to make sure we only clean up truly unused stuff.
There was a problem hiding this comment.
We can't - well, we technically can, but that would be an unreasonable thing to automate. For the cleanup to trigger, one has to manually remove the setting from the seed file - at which point it's entirely on them to ensure there's nothing referencing the setting in code, we can't do anything about that. And if we remove a setting (as is the case with MaxUploadSize here), the onus is on us to update the codebase to not use it anymore.
There was a problem hiding this comment.
Ah, ok. I assume grepping the code for the name of the setting you're removing is sufficient to catch any remaining references?
There was a problem hiding this comment.
Yup, there should be no code referencing the old setting (I thoroughly purged it before dropping, of course) but double-checking won't hurt
There was a problem hiding this comment.
My question wasn't about this change, but future ones. :-) Is there a comment in the YAML file to the effect that removing an entry will purge it (not just stop seeding it)? Belt and suspenders isn't bad when it comes to comments for future editors.
There was a problem hiding this comment.
I have hard time imagining someone removing a baked in site setting without checking how it's used and expecting it to not break anything, but I suppose a mention in the docs won't hurt. We might want the cleanup to be locked behind an environment variable similar to UPDATE_POSTS just to be on the safe side (it's not like having a dangling site setting is the end of the world). @ArtOfCode-, what do you think about adding one?
There was a problem hiding this comment.
I'm probably being needlessly paranoid -- that PR would get reviewed, after all, and we'd have a conversation like this if it didn't mention calling code or the lack thereof. I think we can leave it as-is. (Feel free to resolve this conversation.)
|
For #1884, none is needed as long as the new timestamp format works. For #1671 one could tweak the new site setting ( |
cellio
left a comment
cellio
left a comment
There was a problem hiding this comment.
Tested everything listed (and checked off) in a previous comment and it all looks good. I have a (paranoid?) question about deleted posts, and someone more qualified should review new JS and the JSON & config changes in the db directory. (I looked at all the other code changes and they look fine to me.)
trichoplax
left a comment
trichoplax
left a comment
There was a problem hiding this comment.
I have a couple of requests and a question.
3 participants
closes #1968 (the label is now configurable via locale strings):
closes #1971:
closes #1975 - comment threads now use the
priority_ordercommon scope regardless of whether they are initially loaded or expanded via the "show more" button (note that we still order byupdated_atand notlast_activity_at- we should switch to the latter when we can).closes #1970:
closes #1965 (see below);
closes #1966 (see below);
closes #1884 (we now simply use Moment.js to format timestamps - note the new format and preserved relative time):
closes #1977 (also exposes
MaxUploadSizeto client-side code asQPixel.MAX_UPLOAD_SIZE):closes #1969 (with existing threads and no threads respectively):
closes #1671 (new site setting:
MaxRequestBodySize):