This commit addresses an issue for sites where secure_uploads
is turned on after the site has been operating without it for
some time.
When uploads are linked when they are used inside a post,
we were setting the access_control_post_id unconditionally
if it was NULL to that post ID and secure_uploads was true.
However this causes issues if an upload has been used in a
few different places, especially if a post was previously
used in a PM and marked secure, so we end up with a case of
the upload using a public post for its access control, which
causes URLs to not use the /secure-uploads/ path in the post,
breaking things like image uploads.
We should only set the access_control_post_id if the post is the first time the
upload is referenced so it cannot hijack uploads from other places.
When lazy load categories is enabled, categories should be loaded with
user activity items and drafts because the categories may not be
preloaded on the client side.
This will automatically enable the glimmer header when all installed themes/plugins are ready. This replaces the old group-based site setting.
In 'auto' mode, we check for calls to deprecated APIs (e.g. decorateWidget) which affect the old header. If any are present, we stick to the old header implementation and print a message to the console alongside the normal deprecation messages.
To override this automatic behavior, a new `glimmer_header_mode` site setting can be set to 'disabled' or 'enabled'.
This change also means that our test suite is running with the glimmer header. This unveiled a couple of small issues (e.g. some incorrect `aria-*` and `alt` text) which are now fixed. A number of selectors had to be updated to ensure the tests were clicking the actual `<button>` elements rather than the surrounding `<li>` elements.
This modifier allows plugins to alter the outcome of
`should_secure_uploads?` on a Post record, for cases when
plugins need post-attached uploads to always be secure (or
not secure) in specific scenarios.
This method name is a bit confusing; with_secure_uploads implies
it may return a block or something with the uploads of the post,
and has_secure_uploads implies that it's checking whether the post
is linked to any secure uploads.
should_secure_uploads? communicates the true intent of this method --
which is to say whether uploads attached to this post should be
secure or not.
* DEV: Add `topic_embed_import_create_args` plugin modifier
This modifier allows a plugin to change the arguments used when creating
a new topic for an imported article.
For example: let's say you want to prepend "Imported: " to the title of
every imported topic. You could use this modifier like so:
```ruby
# In your plugin's code
plugin.register_modifier(:topic_embed_import_create_args) do |args|
args[:title] = "Imported: #{args[:title]}"
args
end
```
In this example, the modifier is prepending "Imported: " to the `title` in the `create_args` hash. This modified title would then be used when the new topic is created.
This PR improves the performance of the `most_replied_to_users` method on the `UserSummary` model.
### Old Query
```ruby
post_query
.joins(
"JOIN posts replies ON posts.topic_id = replies.topic_id AND posts.reply_to_post_number = replies.post_number",
)
# We are removing replies by @user, but we can simplify this by getting the using the user_id on the posts.
.where("replies.user_id <> ?", @user.id)
.group("replies.user_id")
.order("COUNT(*) DESC")
.limit(MAX_SUMMARY_RESULTS)
.pluck("replies.user_id, COUNT(*)")
.each { |r| replied_users[r[0]] = r[1] }
```
### Old Query with corrections
```ruby
post_query
.joins(
"JOIN posts replies ON posts.topic_id = replies.topic_id AND replies.reply_to_post_number = posts.post_number",
)
# Remove replies by @user but instead look on loaded posts (we do this so we don't count self replies)
.where("replies.user_id <> posts.user_id")
.group("replies.user_id")
.order("COUNT(*) DESC")
.limit(MAX_SUMMARY_RESULTS)
.pluck("replies.user_id, COUNT(*)")
.each { |r| replied_users[r[0]] = r[1] }
```
### New Query
```ruby
post_query
.joins(
"JOIN posts replies ON posts.topic_id = replies.topic_id AND posts.reply_to_post_number = replies.post_number",
)
# Only include regular posts in our joins, this makes sure we don't have the bloat of loading private messages
.joins(
"JOIN topics ON replies.topic_id = topics.id AND topics.archetype <> 'private_message'",
)
# Only include visible post types, so exclude posts like whispers, etc
.joins(
"AND replies.post_type IN (#{Topic.visible_post_types(@user, include_moderator_actions: false).join(",")})",
)
.where("replies.user_id <> posts.user_id")
.group("replies.user_id")
.order("COUNT(*) DESC")
.limit(MAX_SUMMARY_RESULTS)
.pluck("replies.user_id, COUNT(*)")
.each { |r| replied_users[r[0]] = r[1] }
```
# Conclusion
`most_replied_to_users` was untested, so I introduced a test for the logic, and have confirmed that it passes on both the new query **AND** the old query.
Thank you @danielwaterworth for the debugging assistance.
We will be collecting the logo URL and the site's default locale values along with existing basic details to display the site on the Discourse Discover listing page. It will be included only if the site is opted-in by enabling the "`include_in_discourse_discover`" site setting.
Also, we no longer going to use `about.json` and `site/statistics.json` endpoints retrieve these data. We will be using only the `site/basic-info.json` endpoint.
When a user is manually deactivated, they should not be deleted by our background job that purges inactive users.
In addition, site settings keywords should accept an array of keywords.
Previously the problem check registry simply looked at the subclasses of ProblemCheck. This was causing some confusion in environments where eager loading is not enabled, as the registry would appear empty as a result of the classes never being referenced (and thus never loaded.)
This PR changes the approach to a more explicit one. I followed other implementations (bookmarkable and hashtag autocomplete.) As a bonus, this now has a neat plugin entry point as well.
Why this change?
This is a follow up to 897be75941.
When updating `net-smtp` from `0.4.x` to `0.5.x`, our test suite passed
but the error `ArgumentError: SMTP-AUTH requested but missing user name`
was being thrown in production leading to emails being failed to send
out via SMTP.
This commit adds a test to ensure that our production SMTP settings will
at least attemp to connect to an SMTP server.
## Why this change?
The previous implementation of the method generated the query to find the relevant topics and iterated over the results, processing them.
This behavior made difficult reusing or changing the query logic in classes extending `CategoryList`.
This commit extracts the query logic into another method called `relevant_topics_query ` which can be reused or overwritten in descendant classes.
This was originally introduced in #26071, but that PR was closed, because the requirements changed. This PR lifts only the relevant parts, since they are a prerequisite for the new admin notice system.
This enables the following in Discourse AI
```
plugin.register_modifier(:chat_allowed_bot_user_ids) do |user_ids, guardian|
if guardian.user
mentionables = AiPersona.mentionables(user: guardian.user)
allowed_bot_ids = mentionables.map { |mentionable| mentionable[:user_id] }
user_ids.concat(allowed_bot_ids)
end
user_ids
end
```
some bots that are id < 0 need to be discoverable in search otherwise people can not talk to them.
---------
Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>
When "lazy load categories" is enabled and parent_category_id was set,
the query fetching categories contained a contradiction filtering both
by parent_category_id and parent_category_id = NULL.
When categories are loaded by the frontend, the parent category is
looked up by ID and the `parentCategory` is set with the result. If the
categories returned are not in order, the parent category may miss.
We were using `OFFSET`/`LIMIT` to query topics without an 'ORDER'. Without an explicit order, postgres makes no guarantees about which rows will be returned for each query. This commit adds `ORDER BY id ASC` so that our sitemaps behave consistently.
All our link validation, and conversion from url -> route/model/query is expensive and prone to bugs. Instead, if people enter a link, we can just use it as-is.
Originally all this extra logic was added to handle unusual situations like `/safe-mode`, `/my/...`, etc. However, all of these are now handled correctly by our Ember router, so there is no need for it.
Now, we just pass the user-supplied `href` directly to the SectionLink component, and let Ember handle routing to it when clicked.
The only functional change here is that we no longer validate internal links by parsing them with the Ember router. But I'd argue this is fine, because the previous logic would cause both false positives (e.g. `/t/123` would be valid, even if topic 123 doesn't exist), and false negatives (for routes which are server-side only, like the new AI share pages).
We were incorrectly using `return` in a block which was causing exceptions at runtime. These exceptions were not causing much issues as they are in defer block.
While working on writing a test for this specific case, I noticed that our `upsert_custom_fields` function was using rails `update_all` which is not updating the `updated_at` timestamp. This commit also fixes it and adds a test for it.
In #26122 we promoted all problem checks defined as class methods on AdminDashboardData to their own first-class ProblemCheck instances.
This PR continues that by promoting problem checks that are implemented as blocks as well. This includes updating a couple plugins that have problem checks.
This is a follow up to e2da72b76c.
Why this change?
According to https://web.dev/articles/preload-critical-assets,
> By preloading a certain resource, you are telling the browser that you would like to fetch it sooner than the browser would otherwise discover it because you are certain that it is important for the current page.
The preload resource hint is meant to tell the browser to fetch
resources that it would not discover upfront or early. However, we are
not using it the right way because we are literally adding the resource
hint right before a `<script>` tag which means the browser would have
discovered the resource even without the resource hint.
What does this change do?
This commit removes the preload resource hint which are added right
before script tags since the optimization here is highly questionable at the expense of making
our initial DOM larger.
We never use that information and this also fixes an issue with the BCC plugin which ends up triggering a rate-limit because we were publishing a "NEW_PRIVATE_MESSAGE" to the user sending the BCC for every recipients 💥
Internal - t/118283
This commit operates at three levels of abstraction:
1. We want to prevent user history rows from being unbounded in size.
This commit adds rails validations to limit the sizes of columns on
user_histories,
2. However, we don't want to prevent certain actions from being
completed if these columns are too long. In those cases, we truncate
the values that are given and store the truncated versions,
3. For endpoints that perform staff actions, we can further control
what is permitted by explicitly validating the params that are given
before attempting the action,
In AdminDashboardData we have a bunch of problem checks implemented as methods on that class. This PR absolves it of the responsibility by promoting each of those checks to a first class ProblemCheck. This way each of them can have their own priority and arbitrary functionality can be isolated in its own class.
Think "extract class" refactoring over and over. Since they were all moved we can also get rid of the @@problem_syms class variable which was basically the old version of the registry now replaced by ProblemCheck.realtime.
In addition AdminDashboardData::Problem value object has been entirely replaced with the new ProblemCheck::Problem (with compatible API).
Lastly, I added some RSpec matchers to simplify testing of problem checks and provide helpful error messages when assertions fail.
There are a couple of reasons for this.
The first one is practical, and related to eager loading. Since /lib is not eager loaded, when the application boots, ProblemCheck["identifier"] will be nil because the child classes aren't loaded.
The second one is more conceptual. There turns out to be a lot of inter-dependencies between the part of the problem check system that live in /app and the parts that live in /lib, which probably suggests it should all go in /app.
This change creates a user setting that they can toggle if
they don't want to receive unread notifications when someone closes a
topic they have read and are watching/tracking it.
Why this change?
There are two problematic queries in question here when loading
notifications in various tabs in the user menu:
```
SELECT "notifications".*
FROM "notifications"
LEFT JOIN topics ON notifications.topic_id = topics.id
WHERE "notifications"."user_id" = 1338 AND (topics.id IS NULL OR topics.deleted_at IS NULL)
ORDER BY notifications.high_priority AND NOT notifications.read DESC,
NOT notifications.read AND notifications.notification_type NOT IN (5,19,25) DESC,
notifications.created_at DESC
LIMIT 30;
```
and
```
EXPLAIN ANALYZE SELECT "notifications".*
FROM "notifications"
LEFT JOIN topics ON notifications.topic_id = topics.id
WHERE "notifications"."user_id" = 1338
AND (topics.id IS NULL OR topics.deleted_at IS NULL)
AND "notifications"."notification_type" IN (5, 19, 25)
ORDER BY notifications.high_priority AND NOT notifications.read DESC, NOT notifications.read DESC, notifications.created_at DESC LIMIT 30;
```
For a particular user, the queries takes about 40ms and 26ms
respectively on one of our production instance where the user has 10K notifications while the site has 600K notifications in total.
What does this change do?
1. Adds the `index_notifications_user_menu_ordering` index to the `notifications` table which is
indexed on `(user_id, (high_priority AND NOT read) DESC, (NOT read)
DESC, created_at DESC)`.
1. Adds a second index `index_notifications_user_menu_ordering_deprioritized_likes` to the `notifications`
table which is indexed on `(user_id, (high_priority AND NOT read) DESC, (NOT read AND notification_type NOT IN (5,19,25)) DESC, created_at DESC)`. Note that we have to hardcode the like typed notifications type here as it is being used in an ordering clause.
With the two indexes above, both queries complete in roughly 0.2ms. While I acknowledge that there will be some overhead in insert,update or delete operations. I believe this trade-off is worth it since viewing notifications in the user menu is something that is at the core of using a Discourse forum so we should optimise this experience as much as possible.
Why this change?
Prior to this change, the `CategoryList#find_relevant_topics` method was
loading and allocating all `CategoryFeaturedTopic` records in the
database to eventually only just use its `category_id` and `topic_id`
column. On a site with many `CategoryFeaturedTopic` records, the loading
of the ActiveRecord objects is a source of bottleneck.
The other problem with the `CategoryList#find_relevant_topics` method is
that it is unconditionally loading all records from the database even if
the user does not have access to the category. This again is wasteful.
What does this change do?
This commit makes it such that `CategoryList#find_relevant_topics` is
called only after `CategoryList#find_categories` in the `CategoryList#initialize`
method so that we can filter featured topics against categories that the
user has access to.
The second change is that Instead of loading `CategoryFeaturedTopic` records, we make an
inner join agains the `topics` table instead and skip any allocation of
`CatgoryFeaturedTopic` ActiveRecord objects.
As part of problem checks refactoring, we're moving some data to be DB backed. In this PR it's the tracking of problem check execution. When was it last run, when was the last problem, when should it run next, how many consecutive checks had problems, etc.
This allows us to implement the perform_every feature in scheduled problem checks for checks that don't need to be run every 10 minutes.
Users can hide their public profile and presence information by checking
“Hide my public profile and presence features” on the
`u/{username}/preferences/interface` page. In that case, we also don't
want to return user status from the server.
This work has been started in https://github.com/discourse/discourse/pull/23946.
The current PR fixes all the remaining places in Core.
Note that the actual fix is quite simple – a5802f484d.
But we had a fair amount of duplication in the code responsible for
the user status serialization, so I had to dry that up first. The refactoring
as well as adding some additional tests is the main part of this PR.
Now forums can enroll their sites to be showcased in the Discourse [Discover](https://discourse.org/discover) directory. Once they enable the site setting `include_in_discourse_discover` to enroll their forum the `CallDiscourseHub` job will ping the `api.discourse.org/api/discover/enroll` endpoint. Then the Discourse Hub will fetch the basic details from the forum and add it to the review queue. If the site is approved then the forum details will be displayed in the `/discover` page.
Also, remove experimental setting and simply use top_menu for feature detection
This means that when people eventually enable the hot top menu, there will
be topics in it
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Previously, problem checks were all added as either class methods or blocks in AdminDashboardData. Another set of class methods were used to add and run problem checks.
As of this PR, problem checks are promoted to first-class citizens. Each problem check receives their own class. This class of course contains the implementation for running the check, but also configuration items like retry strategies (for scheduled checks.)
In addition, the parent class ProblemCheck also serves as a registry for checks. For example we can get a list of all existing check classes through ProblemCheck.checks, or just the ones running on a schedule through ProblemCheck.scheduled.
After this refactor, the task of adding a new check is significantly simplified. You add a class that inherits ProblemCheck, you implement it, add a test, and you're good to go.
Why this change?
Firstly, note that this is not a security commit because this feature is
still in development and should not be used anywhere.
The reason we want to set a limit here is to greatly reduce the
possibility of a DoS attack in the future via `ThemeSetting` where
someone would set an arbituary large json string in
`ThemeSetting#json_value` and causing the server to run out of resources
trying to serialize/deserialize the value.
What does this change do?
Adds an ActiveRecord validation to ensure that the bytesize of the json
string being stored is smaller than or equal to 0.5mb. We believe 0.5mb
is a decent limit for now but we can review the limit in the future if
we believe it is too small.
Why this change?
The logic for validating a theme setting's value and default value was
not consistent as each part of the code would implement its own logic.
This is not ideal as the default value may be validated differently than
when we are setting a new value. Therefore, this commit seeks to
refactor all the validation logic for a theme setting's value into a
single service class.
What does this change do?
Introduce the `ThemeSettingsValidator` service class which holds all the
necessary helper methods required to validate a theme setting's value
When "lazy load categories" is enabled, only the categories present in
the sidebar are preloaded. This is insufficient because the parent
categories are necessary too for the sidebar to be rendered properly.
The strict-dynamic CSP directive is supported in all our target browsers, and makes for a much simpler configuration. Instead of allowlisting paths, we use a per-request nonce to authorize `<script>` tags, and then those scripts are allowed to load additional scripts (or add additional inline scripts) without restriction.
This becomes especially useful when admins want to add external scripts like Google Tag Manager, or advertising scripts, which then go on to load a ton of other scripts.
All script tags introduced via themes will automatically have the nonce attribute applied, so it should be zero-effort for theme developers. Plugins *may* need some changes if they are inserting their own script tags.
This commit introduces a strict-dynamic-based CSP behind an experimental `content_security_policy_strict_dynamic` site setting.
Reactions needs this to be able to filter out likes received
actions, where there is also an associated reaction, since
now most reactions also count as a like.
The `-ping` option significantly speeds up the ImageMagick `identify` command per our testing and the [documentation](https://imagemagick.org/script/command-line-options.php#ping):
> -ping
Efficiently determine these image characteristics: image number, the file name, the width and height of the image, whether the image is colormapped or not, the number of colors in the image, the number of bytes in the image, the format of the image (JPEG, PNM, etc.). Use +ping to ensure accurate image properties.
We already pass the `-ping` option in other places where the `identify` command is used, so it makes sense to use the option everywhere.
Internal topic: t/121431.
This would allow a theme component (or an API call) to reset the bump
date of a topic to a given post's created_at date.
I picked `post_id` as the parameter here because it provides a bit of
extra protection against accidentally resetting the bump date to a date
that doesn't make sense.
When we insert into the hot set we add things with a score of 0
This means that if hot has more than batch size items in it with a score, then the 0s don't get an initial score
This corrects the situation by always ensuring we re-score:
1. batch size high scoring topics
2. (new) batch size recently bumped topics
* Update spec/models/topic_hot_scores_spec.rb
Co-authored-by: Isaac Janzen <50783505+janzenisaac@users.noreply.github.com>
---------
Co-authored-by: Isaac Janzen <50783505+janzenisaac@users.noreply.github.com>
Removes duplication from LimitedEdit to see who can edit
posts, and also removes the old trust level setting check
since it's no longer necessary.
Also make it so staff can always edit since can_edit_post?
already has a staff escape hatch.
Why this change?
This commit introduces an experimental `type: objects` theme setting
which will allow theme developers to store a collection of objects as
JSON in the database. Currently, the feature is still in development and
this commit is simply setting up the ground work for us to introduce the
feature in smaller pieces.
What does this change do?
1. Adds a `json_value` column as `jsonb` data type to the `theme_settings` table.
2. Adds a `experimental_objects_type_for_theme_settings` site setting to
determine whether `ThemeSetting` records of with the `objects` data
type can be created.
3. Updates `ThemeSettingsManager` to support read/write access from the
`ThemeSettings#json_value` column.
Affects the following settings:
* whispers_allowed_groups
* anonymous_posting_allowed_groups
* personal_message_enabled_groups
* shared_drafts_allowed_groups
* here_mention_allowed_groups
* uploaded_avatars_allowed_groups
* ignore_allowed_groups
This turns off `client: true` for these group-based settings,
because there is no guarantee that the current user gets all
their group memberships serialized to the client. Better to check
server-side first.
Why this change?
This is caused by a regression in
59839e428f, where we stopped saving the
`Theme` object because it was unnecessary. However, it resulted in the
`after_save` callback not being called and hence
`Theme#update_javascript_cache!` not being called. As a result, some
sites were reporting that after runing a theme migration, the defaults
for the theme settings were used instead of the settings overrides
stored in the database.
What does this change do?
Add a call to `Theme#update_javascript_cache!` after running theme
migrations.
This fixes a bug where the sidebar categories would not be loaded when
the categories were lazy loaded because the sidebar uses the preloaded
category list, which was empty.
We just completed the 3.2 release, which marks a good time to drop some previously deprecated columns.
Since the column has been marked in ignored_columns, it has been inaccessible to application code since then. There's a tiny risk that this might break a Data Explorer query, but given the nature of the column, the years of disuse, and the fact that such a breakage wouldn't be critical, we accept it.
1. Don't show visited line for hot filter, it is in random order
2. Don't count likes on non regular posts (eg: whispers / small actions)
3. Don't count participants in non regular posts
1. Serial likers will just like a bunch of posts on the same topic, this will
heavily inflate hot score. To avoid artificial "heat" generated by one user only count
the first like on the topic within the recent_cutoff range per topic
2. When looking at recent topics prefer "unique likers", defer to total likes on
older topics cause we do not have an easy count for unique likers
3. Stop taking 1 off like_count, it is not needed - platforms like reddit
allow you to like own post so they need to remove it.
Why this change?
Returning an array makes it hard to immediately retrieve a setting by
name and makes the retrieval an O(N) operation. By returning an array,
we make it easier for us to lookup a setting by name and retrieval is
O(1) as well.
The `deprecate_column` helper would change its behavior based on the current `Discourse::VERSION`. This means that 'finalizing' a stable release introduces a previously untested behavior change.
Much better to keep it as a deprecation until manual action is taken to introduce the breaking change.
Internal links always notify and add internal connections in topics.
This adds a special feature that lets you append `?silent=true` to a link
to have it excluded from:
1. Notifications - users will not be notified for these links
2. Post links below posts in the UI
This is specifically useful for large reports where adding all these connections
just results in noise.
Running Discourse 3.2 stable under Ember 3 will technically be possible, but is only intended as a short-term migration point. This commit adds an admin warning for sites which are using this configuration, to make it clear that themes and plugins are unlikely to support the configuration.
https://meta.discourse.org/t/287211
We want to exclude the system user from group user counts, since intuitively admins wouldn't include them.
Originally this was accomplished by booting said system user from the groups, but this is causing problems, because the system user needs TL group membership to perform certain tasks.
After this PR, system user is still in the TL groups, but excluded when refreshing the user count.
