Previously we would only recompile a theme locale when its own data changes. However, the output also includes fallback data from other locales, so we need to invalidate all locales when fallback locale data is changed. Building a list of dependent locales is tricky, so let's just invalidate them all.
Followup to fe05fdae24
For consistency with other S3 settings, make the global setting
the same name as the site setting and use SiteSetting.Upload
too so it reads from the correct place.
This adds the ability to collect stats without exposing them
among other stats via API.
The most important thing I wanted to achieve is to provide
an API where stats are not exposed by default, and a developer
has to explicitly specify that they should be
exposed (`expose_via_api: true`). Implementing an opposite
solution would be simpler, but that's less safe in terms of
potential security issues.
When working on this, I had to refactor the current solution.
I would go even further with the refactoring, but the next steps
seem to be going too far in changing the solution we have,
and that would also take more time. Two things that can be
improved in the future:
1. Data structures for holding stats can be further improved
2. Core stats are hard-coded in the About template (it's hard
to fix it without correcting data structures first, see point 1):
63a0700d45/app/views/about/index.html.erb (L61-L101)
The most significant refactorings are:
1. Introducing the `Stat` model
2. Aligning the way the core and the plugin stats' are registered
There was a registry for preloaded site categories and a new one has
been introduced recently for categories serialized through a
CategoryList.
Having two registries created a lot of friction for developers and this
commit merges them into a single one, providing a unified API.
There is an edge case where the following occurs:
1. The user sets a bookmark reminder on a post/topic
2. The post/topic is changed to a PM before or after the reminder
fires, and the notification remains unread by the user
3. The user opens their bookmark reminder notification list
and they can still see the notification even though they cannot
access the topic anymore
There is a very low chance for information leaking here, since
the only thing that could be exposed is the topic title if it
changes to something sensitive.
This commit filters the bookmark unread notifications by using
the bookmarkable can_see? methods and also prevents sending
reminder notifications for bookmarks the user can no longer see.
When quoting a chat message in a post, if that message contains a mention,
that mention should be ignored. But we've been detecting them and sending
notifications to users. This PR fixes the problem. Since this fix is for
the chat plugin, I had to introduce a new API for plugins:
# We strip posts before detecting mentions, oneboxes, attachments etc.
# We strip those elements that shouldn't be detected. For example,
# a mention inside a quote should be ignored, so we strip it off.
# Using this API plugins can register their own post strippers.
def register_post_stripper(&block)
end
We updated scheduled admin checks to run concurrently in their own jobs. The main reason for this was so that we can implement re-check functionality for especially flaky checks (e.g. group e-mail credentials check.)
This works in the following way:
1. The check declares its retry policy using class methods.
2. A block can be yielded to if there are problems, but before they are committed to Redis.
3. The job uses this block to either a) schedule a retry if there are any remaining or b) do nothing and let the check commit.
This PR does some preparatory refactoring of scheduled admin checks in order for us to be able to do custom retry strategies for some of them.
Instead of running all checks in sequence inside a single, scheduled job, the scheduled job spawns one new job per check.
In order to be concurrency-safe, we need to change the existing Redis data structure from a string (of serialized JSON) to a list of strings (of serialized JSON).
This commit introduces a new feature that allows theme developers to manage the transformation of theme settings over time. Similar to Rails migrations, the theme settings migration system enables developers to write and execute migrations for theme settings, ensuring a smooth transition when changes are required in the format or structure of setting values.
Example use cases for the theme settings migration system:
1. Renaming a theme setting.
2. Changing the data type of a theme setting (e.g., transforming a string setting containing comma-separated values into a proper list setting).
3. Altering the format of data stored in a theme setting.
All of these use cases and more are now possible while preserving theme setting values for sites that have already modified their theme settings.
Usage:
1. Create a top-level directory called `migrations` in your theme/component, and then within the `migrations` directory create another directory called `settings`.
2. Inside the `migrations/settings` directory, create a JavaScript file using the format `XXXX-some-name.js`, where `XXXX` is a unique 4-digit number, and `some-name` is a descriptor of your choice that describes the migration.
3. Within the JavaScript file, define and export (as the default) a function called `migrate`. This function will receive a `Map` object and must also return a `Map` object (it's acceptable to return the same `Map` object that the function received).
4. The `Map` object received by the `migrate` function will include settings that have been overridden or changed by site administrators. Settings that have never been changed from the default will not be included.
5. The keys and values contained in the `Map` object that the `migrate` function returns will replace all the currently changed settings of the theme.
6. Migrations are executed in numerical order based on the XXXX segment in the migration filenames. For instance, `0001-some-migration.js` will be executed before `0002-another-migration.js`.
Here's a complete example migration script that renames a setting from `setting_with_old_name` to `setting_with_new_name`:
```js
// File name: 0001-rename-setting.js
export default function migrate(settings) {
if (settings.has("setting_with_old_name")) {
settings.set("setting_with_new_name", settings.get("setting_with_old_name"));
}
return settings;
}
```
Internal topic: t/109980
The User#flag_level column has not been in use for a very long time. The "new" reviewable system dynamically calculates flag scores based on past performance of the user.
This PR removes flag_level from the admin user serializer (since it isn't displayed anywhere in admin user lists) and marks the column as deprecated and targeted for removal in the next minor version.
The message: :signup_not_allowed option to the IP address validator does nothing, because the AllowedIpAddressValidator chooses one of either:
- ip_address.blocked or
- ip_address.max_new_accounts_per_registration_ip
internally. This means that the translation for this was also never used.
This PR removes the ineffectual option and the unused translation. It also moves the translated error messages for blocked and max_new_accounts_per_registration_ip into the correct location so we can pass a symbol to ActiveModel::Errors#add.
There is no actual change in behaviour.
Followup to 9762e65758. This
original commit did not take into account the fact that
new topics can end up in the approval queue as a
ReviewableQueuedPost, and so there was a 500 error raised
when accessing `self.topic` when sending a PM to the user.
Using SiteSetting.queue_jobs= to configure job asynchronicity was deprecated here four years ago and marked for removal in version 2.9.0. This PR removes the fallback method we kept since then. The method was there because it was still being used in a bunch of plugin tests (now fixed.)
The PostAction.remove_act class method has been deprecated and replaced by PostActionDestroyer. It was marked for removal in version 2.9.0. This PR removes the method.
Why this change?
Currently, we do not have a method to easily retrieve a theme setting's
value on the server side. Such a method can be useful in the test
environment where we need to retrieve the theme's setting and use its
value in assertions.
What does this change do?
This change introduces the `Theme#get_setting` instance method.
This change adds a new event trigger (new_post_moved) when the first post in a topic is moved to a new topic.
Plugins that listen for the new_post_moved event now have an easy way to update old data based on the post id.
There are a few PUT requests that users can do in their preferences tab that aren't going through the standard `user#update` action.
This commit adds all the "trivial" ones (aka. except the security-related one, username and email changes) so you can now change the badge title, the avatar or featured topic of a user via the API.
This commit adds a new admin UI under the route `/admin-revamp`, which is
only accessible if the user is in a group defined by the new `enable_experimental_admin_ui_groups` site setting. It
also adds a special `admin` sidebar panel that is shown instead of the `main`
forum one when the admin is in this area.
We also add an "Admin Revamp" sidebar link to the community section, which
will only appear if the user is in the setting group:
Within this there are subroutes defined like `/admin-revamp/config/:area`,
these areas could contain any UI imaginable, this is just laying down an
initial idea of the structure and how the sidebar will work. Sidebar links are
currently hardcoded.
Some other changes:
* Changed the `main` and `chat` panels sidebar panel keys to use exported const values for reuse
* Allowed custom sidebar sections to hide their headers with the `hideSectionHeader` option
* Add a `groupSettingArray` setting on `this.siteSettings` in JS, which accepts a group site setting name
and splits it by `|` then converts the items in the array to integers, similar to the `_map` magic for ruby
group site settings
* Adds a `hidden` option for sidebar panels which prevents them from showing in separated mode and prevents
the switch button from being shown
---------
Co-authored-by: Krzysztof Kotlarek <kotlarek.krzysztof@gmail.com>
* FIX: Secure upload post processing race condition
This commit fixes a couple of issues.
A little background -- when uploads are created in the composer
for posts, regardless of whether the upload will eventually be
marked secure or not, if secure_uploads is enabled we always mark
the upload secure at first. This is so the upload is by default
protected, regardless of post type (regular or PM) or category.
This was causing issues in some rare occasions though because
of the order of operations of our post creation and processing
pipeline. When creating a post, we enqueue a sidekiq job to
post-process the post which does various things including
converting images to lightboxes. We were also enqueuing a job
to update the secure status for all uploads in that post.
Sometimes the secure status job would run before the post process
job, marking uploads as _not secure_ in the background and changing
their ACL before the post processor ran, which meant the users
would see a broken image in their posts. This commit fixes that issue
by always running the upload security changes inline _within_ the
cooked_post_processor job.
The other issue was that the lightbox wrapper link for images in
the post would end up with a URL like this:
```
href="/secure-uploads/original/2X/4/4e1f00a40b6c952198bbdacae383ba77932fc542.jpeg"
```
Since we weren't actually using the `upload.url` to pass to
`UrlHelper.cook_url` here, we weren't converting this href to the CDN
URL if the post was not in a secure context (the UrlHelper does not
know how to convert a secure-uploads URL to a CDN one). Now we
always end up with the correct lightbox href. This was less of an issue
than the other one, since the secure-uploads URL works even when the
upload has become non-secure, but it was a good inconsistency to fix
anyway.
This reverts commit 5f0bc4557f.
Through extensive internal discussion we have decided to revert
this change, as it significantly impacted moderation flow for
some Discourse site moderators, especially around "something else"
flags. We need to re-approach how flags are counted holistically,
so to that end this change is being reverted.
Site data is preloaded on the first page load, which includes categories
data. For sites with many categories, site data takes a long time to
serialize and to transfer.
In the future, preloaded category data will be completely removed.
The category style site setting is being deprecated. This commit will
show a warning on the admin dashboard if a site isn't using the default
category style (bullet).
At this moment, this feature is under a site setting named
lazy_load_categories.
In the future, categories will no longer be preloaded through site data.
This commit add information about categories in topic list and ensures
that data is used to display topic list items.
Parent categories are serialized too because they are necessary to
render {{category-link}}.
There are cases where a user can copy image markdown from a public
post (such as via the discourse-templates plugin) into a PM which
is then sent via an email. Since a PM is a secure context (via the
.with_secure_uploads? check on Post), the image will get a secure
URL in the PM post even though the backing upload is not secure.
This fixes the bug in that case where the image would be stripped
from the email (since it had a /secure-uploads/ URL) but not re-attached
further down the line using the secure_uploads_allow_embed_images_in_emails
setting because the upload itself was not secure.
The flow in Email::Sender for doing this is still not ideal, but
there are chicken and egg problems around when to strip the images,
how to fit in with other attachments and email size limits, and
when to apply the images inline via Email::Styles. It's convoluted,
but at least this fixes the Template use case for now.
Why this change?
The `PostsController#create` action allows arbitrary topic custom fields
to be set by any user that can create a topic. Without any restrictions,
this opens us up to potential security issues where plugins may be using
topic custom fields in security sensitive areas.
What does this change do?
1. This change introduces the `register_editable_topic_custom_field` plugin
API which allows plugins to register topic custom fields that are
editable either by staff users only or all users. The registered
editable topic custom fields are stored in `DiscoursePluginRegistry` and
is called by a new method `Topic#editable_custom_fields` which is then
used in the `PostsController#create` controller action. When an unpermitted custom fields is present in the `meta_data` params,
a 400 response code is returned.
2. Removes all reference to `meta_data` on a topic as it is confusing
since we actually mean topic custom fields instead.
This commit adds a new Revise... action that can be taken
for queued post reviewables. This will open a modal where
the user can select a Reason from a preconfigured list
(or by choosing Other..., a custom reason) and provide feedback
to the user about their post.
The post will be rejected still, but a PM will also be sent to
the user so they have an opportunity to improve their post when
they resubmit it.
We run the ember-this-fallback transformation on plugin and theme code so that they can continue omitting `this.` in `.hbs` templates. A bug in the implementation meant that it was incorrectly transforming things like `{{dir/some-component}}` into `<DirSomeComponent />` (rather than `<Dir::SomeComponent />`).
This commit uses patch-package to apply the fix from https://github.com/tildeio/ember-this-fallback/pull/56
When a user creates or edits a post, we already were updating
the security of uploads in the post based on site settings and
their access control post, which is important since these uploads
may be switched from secure/not secure based on configuration.
The `with_secure_uploads?` method on a post is used to determine
whether to use the secure-uploads URL for all uploads in the post,
regardless of their individual security, so if this is false and
some of the posts are still secure when rebaking, we end up with
broken URLs.
This commit just makes it so rebaking via the UI also re-evaluates
upload security so that when the post is loaded again after processing,
all of the uploads have the correct security.
Why this change?
Back in May 17 2023 along with the release of Discourse 3.1, we announced
on meta that the legacy hamburger dropdown navigation menu is
deprecated and will be dropped in Discourse 3.2. This is the link to the announcement
on meta: https://meta.discourse.org/t/removing-the-legacy-hamburger-navigation-menu-option/265274
## What does this change do?
This change removes the `legacy` option from the `navigation_menu` site
setting and migrates existing sites on the `legacy` option to the
`header dropdown` option.
All references to the `legacy` option in code and tests have been
removed as well.
This is part 1 of 3, split up of PR #23529. This PR refactors the
webauthn code to support passkey authentication/registration.
Passkeys aren't used yet, that is coming in PRs 2 and 3.
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Some sites have a large number of categories and fetching the category
IDs or category topic IDs just to build another query can take a long
time or resources (i.e. memory).
Meta topic: https://meta.discourse.org/t/suppress-these-tags-from-summary-emails-settings-is-not-working-in-preview-digest-email/279196?u=osama
Follow-up to 477a5dd371
The `digest_suppress_tags` setting is designed to be a list of pipe-delimited tag names, but the tag-based topic suppression logic assumes (incorrectly) that the setting contains pipe-delimited tag IDs. This mismatch in expectations led to the setting not working as expected.
This PR adds a step that converts the list of tag names in the setting to their corresponding IDs, which is then used to suppress topics tagged with those specific tags.
This commit adds limits to themes and theme components on the:
- file size of about.json and .discourse-compatibility
- file size of theme assets
- number of files in a theme
The hidden site setting max_drafts_per_user defaults to 10_000 drafts per user.
The longest key should be "topic_<MAX_BIG_INT>" which is 25 characters.
In #20135 we prevented invalid inputs from being accepted in category setting form fields on the front-end. We didn't do anything on the back-end at that time, because we were still discussing which path we wanted to take. Eventually we decided we want to move this to a new CategorySetting model.
This PR moves the require_topic_approval and require_reply_approval from custom fields to the new CategorySetting model.
This PR is nearly identical to #20580, which migrated num_auto_bump_daily, but since these are slightly more sensitive, they are moved after the previous one is verified.
Why this change?
Currently, we do not have an easy way to test themes and theme components
using Rails system tests. While we support QUnit acceptance tests for
themes and theme components, QUnit acceptance tests stubs out the server
and setting up the fixtures for server responses is difficult and can lead to a
frustrating experience. System tests on the other hand allow authors to
set up the test fixtures using our fabricator system which is much
easier to use.
What does this change do?
In order for us to allow authors to run system tests with their themes
installed, we are adding a `upload_theme` helper that is made available
when writing system tests. The `upload_theme` helper requires a single
`directory` parameter where `directory` is the directory of the theme
locally and returns a `Theme` record.
This adds a new secure_uploads_pm_only site setting. When secure_uploads
is true with this setting, only uploads created in PMs will be marked
secure; no uploads in secure categories will be marked as secure, and
the login_required site setting has no bearing on upload security
either.
This is meant to be a stopgap solution to prevent secure uploads
in a single place (private messages) for sensitive admin data exports.
Ideally we would want a more comprehensive way of saying that certain
upload types get secured which is a hybrid/mixed mode secure uploads,
but for now this will do the trick.
They're both constant per-instance values, there is no need to store them
in the session. This also makes the code a bit more readable by moving
the `session_challenge_key` method up to the `DiscourseWebauthn` module.
Follow-up to #23199 in which we moved the "delete user" options under the relevant action menu for flagged post. This change does the same, but to queued posts.
As per discussion, we want to move the options to delete the user under the "Yes" menu of the review queue, since these options are often the most recommended, but also frequently missed because they are tucked away under their own menu.
In addition to the move, I removed the title case of the options so that it matches the other options in the "Yes" menu.
Reverts e2705df and re-lands #23187 and #23219.
The issue was incorrect order of execution of Rails' `assets:precompile` task in our own precompilation stack.
Co-authored-by: David Taylor <david@taylorhq.com>
Manipulating theme module paths means that the paths you author are not the ones used at runtime. This can lead to some very unexpected behavior and potential module name clashes. It also meant that the refactor in 16c6ab8661 was unable to correctly match up theme connector js/templates.
While this could technically be a breaking change, I think it is reasonably safe because:
1. Themes are already forced to use relative paths when referencing their own modules (since they're namespaced based on the site-specific id). The only time this might be problematic is when theme tests reference modules in the theme's main `javascripts` directory
2. For things like components/services/controllers/etc. our custom Ember resolver works backwards from the end of the path, so adding `discourse/` in the middle will not affect resolution.
This is a bug that happens only when the current date is less than 90 days from a date on which the time zone transitions into or out of Daylight Savings Time.
In these conditions, bulk invites show the time of day of their expiration as being 1 hour later than the current time.
Whereas it should match the time of day the invite was generated.
This is because the server has not been using the user's timezone in calculating the expiration time of day. This PR fixes issue by considering the user's timezone when doing the date math.
https://meta.discourse.org/t/bulk-invite-logic-to-generate-expire-date-bug/274689
`ReviewableQueuedPost` got refactored a while back to use the more
appropriate `target_created_by` for the user of the post being queued
instead of `created_by`. The change was not extended to the `DELETE
/review/:id` endpoint leading to error responses for a user attempting
to deleting their own queued post.
This fix extends the `Reviewable` lookup implementation in
`ReviewablesController#destroy` and Guardian implementation to account
for this change.
Previously we were discovering plugin outlets by checking first for dedicated template files, and then looking for classes to match them. This doesn't work for components which are entirely defined in JS (e.g. those authored with gjs, or those which are re-exports of a colocated component).
This commit refactors our detection logic to look for both class and template modules in a single pass. It also refactors things so that the modules themselves are required lazily when needd, rather than all being loaded during app boot.
When hiding a post (essentially updating hidden, hidden_at, and hidden_reason_id) our callbacks are running the whole battery of post validations. This can cause the hiding to fail in a number of edge cases. The issue is similar to the one fixed in #11680, but applies to all post validations, none of which should apply when hiding a post.
After some code reading and discussion, none of the validations in PostValidator seem to be relevant when hiding posts, so instead of just skipping unique check, we skip all post validator checks.
* scrub non-a html tags from tag descriptions on create, strips all tags from tag description when displayed in tag hover
* test for tag description links
* UX: basic render-tag test
* UX: fix linting
* UX: fix linting
* fix broken tests
* Update spec/models/tag_spec.rb
Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>
* UX: use has_sanitizable_fields instead of has_scrubbable_fields to ensafen tag.description
---------
Co-authored-by: Penar Musaraj <pmusaraj@gmail.com>
Currently when we decide we're going to drop a column in the future we just mark it with a TODO comment and add it to ignored_columns. This makes it instantly unavailable, and we mostly forget about the TODO in the end. 😬
This change adds a HasDeprecatedColumns concern which offers a little bit more flexibility. We can still simulate the old behaviour by setting drop_from to the current version, but we can also set it to a future version, causing it to raise a deprecation warning until then if used.
This commit removes any logic in the app and in specs around
enable_experimental_hashtag_autocomplete and deletes some
old category hashtag code that is no longer necessary.
It also adds a `slug_ref` category instance method, which
will generate a reference like `parent:child` for a category,
with an optional depth, which hashtags use. Also refactors
PostRevisor which was using CategoryHashtagDataSource directly
which is a no-no.
Deletes the old hashtag markdown rule as well.
This brings the theme development experience (via the discourse_theme cli) closer to the experience of making javascript changes in Discourse core/plugins via Ember CLI. Whenever a change is made to a non-css theme field, all clients will be instructed to immediately refresh via message-bus.
In #20135 we prevented invalid inputs from being accepted in category setting form fields on the front-end. We didn't do anything on the back-end at that time, because we were still discussing which path we wanted to take. Eventually we decided we want to move this to a new CategorySetting model.
This PR moves the num_auto_bump_daily from custom fields to the new CategorySetting model.
In addition it sets the default value to 0, which exhibits the same behaviour as when the value is NULL.
* FIX Add 'Ignored' flags to Moderator Activity report
The Moderator Activity query didn’t include the number of deferred flags in the Flags Reviewed totals. As this number is designed to reflect how many flags a moderator has seen, reviewed, and made a judgement on, the Ignored ones should also be included.
* Apply suggestions from code review
---------
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
FEATURE: Only approved flags for post counters
* Why was this change necessary?
The counters for flagged posts in the user's profile and user index from
the admin view include flags that were rejected, ignored or pending
review. This introduces unnecessary noise. Also the flagged posts
counter in the user's profile includes custom flags which add further
noise to this signal.
* How does it address the problem?
* Modifying User#flags_received_count to return posts with only approved
standard flags
* Refactoring User#number_of_flagged_posts to alias to
User#flags_received_count
* Updating the flagged post staff counter hyperlink to navigate to a
filtered view of that user's approved flagged posts to maintain
consistency with the counter
* Adding system tests for the profile page to cover the flagged posts
staff counter
In the query generated by `TopicTrackingState.report`, there are two
subqueies being executed. The first subquery fetches all the topics
that are new for a given user while the second subquery fetches all the topics with
unread posts for a given user. For the second subquery, there is a
filter `topics.updated_at >= user_stats.first_unread_at` which is used
as a performance optimisation to reduce the number of rows that PG has
to scan through the `topics` table.
However, we started to notice in production that the PG planner doesn't
always execute the filter first to reduce the number of rows that it has
to scan through. Running the following query in one of our production
instance,
```
EXPLAIN ANALYZE
SELECT
DISTINCT topics.id as topic_id,
u.id as user_id,
topics.created_at,
topics.updated_at,
topics.highest_staff_post_number AS highest_post_number,
last_read_post_number,
c.id as category_id,
c.topic_id AS category_topic_id,
tu.notification_level,
us.first_unread_at,
GREATEST(
CASE
WHEN COALESCE(uo.new_topic_duration_minutes, 2880) = -1 THEN u.created_at
WHEN COALESCE(uo.new_topic_duration_minutes, 2880) = -2 THEN COALESCE(
u.previous_visit_at,u.created_at
)
ELSE ('2023-07-31 03:29:45.737630'::timestamp - INTERVAL '1 MINUTE' * COALESCE(uo.new_topic_duration_minutes, 2880))
END, u.created_at, '2023-07-25 15:06:44'
) AS treat_as_new_topic_start_date
FROM topics
JOIN users u on u.id = 13455
JOIN user_stats AS us ON us.user_id = u.id
JOIN user_options AS uo ON uo.user_id = u.id
JOIN categories c ON c.id = topics.category_id
LEFT JOIN topic_users tu ON tu.topic_id = topics.id AND tu.user_id = u.id
WHERE u.id = 13455 AND
topics.updated_at >= us.first_unread_at AND
topics.archetype <> 'private_message' AND
(("topics"."deleted_at" IS NULL AND (tu.last_read_post_number < topics.highest_staff_post_number) AND (COALESCE(tu.notification_level, 1) >= 2)) OR (1=0)) AND
NOT (
COALESCE((select array_agg(tag_id) from topic_tags where topic_tags.topic_id = topics.id), ARRAY[]::int[]) && ARRAY[451,452,453]
) AND
topics.deleted_at IS NULL AND
NOT (
last_read_post_number IS NULL AND
(
topics.category_id IN (SELECT "categories"."id" FROM "categories" LEFT JOIN categories categories2 ON categories2.id = categories.parent_category_id LEFT JOIN category_users ON category_users.category_id = categories.id AND category_users.user_id = 13455 LEFT JOIN category_users category_users2 ON category_users2.category_id = categories2.id AND category_users2.user_id = 13455 WHERE ((category_users.id IS NULL AND COALESCE(category_users2.notification_level, 1) = 0) OR COALESCE(category_users.notification_level, 1) = 0))
AND tu.notification_level <= 1
)
)
```
we get the following
```
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Unique (cost=201606.06..201608.15 rows=76 width=60) (actual time=91.279..91.294 rows=14 loops=1)
-> Sort (cost=201606.06..201606.25 rows=76 width=60) (actual time=91.278..91.284 rows=14 loops=1)
Sort Key: topics.id, topics.created_at, topics.updated_at, topics.highest_staff_post_number, tu.last_read_post_number, c.id, c.topic_id, tu.notification_level, us.first_unread_at, (GREATEST(CASE WHEN (COALESCE(uo.new_topic_duration_minutes, 2880) = '-1'::integer) THEN u.created_at WHEN (COALESCE(uo.new_topic_duration_minutes, 2880) = '-2'::integer) THEN COALESCE(u.previous_visit_at, u.created_at) ELSE ('2023-07-31 03:29:45.73763'::timestamp without time zone - ('00:01:00'::interval * (COALESCE(uo.new_topic_duration_minutes, 2880))::double precision)) END, u.created_at, '2023-07-25 15:06:44'::timestamp without time zone))
Sort Method: quicksort Memory: 26kB
-> Hash Join (cost=97519.51..201603.69 rows=76 width=60) (actual time=87.662..91.268 rows=14 loops=1)
Hash Cond: (topics.id = tu.topic_id)
Join Filter: ((tu.last_read_post_number < topics.highest_staff_post_number) AND ((tu.last_read_post_number IS NOT NULL) OR (NOT (hashed SubPlan 2)) OR (tu.notification_level > 1)))
Rows Removed by Join Filter: 10
-> Nested Loop (cost=1.54..104075.36 rows=3511 width=68) (actual time=0.055..3.609 rows=548 loops=1)
-> Nested Loop (cost=1.13..25.20 rows=1 width=32) (actual time=0.027..0.033 rows=1 loops=1)
-> Nested Loop (cost=0.71..16.76 rows=1 width=28) (actual time=0.020..0.023 rows=1 loops=1)
-> Index Scan using users_pkey on users u (cost=0.42..8.44 rows=1 width=20) (actual time=0.010..0.012 rows=1 loops=1)
Index Cond: (id = 13455)
-> Index Scan using user_stats_pkey on user_stats us (cost=0.29..8.31 rows=1 width=12) (actual time=0.008..0.010 rows=1 loops=1)
Index Cond: (user_id = 13455)
-> Index Scan using index_user_options_on_user_id_and_default_calendar on user_options uo (cost=0.42..8.44 rows=1 width=8) (actual time=0.007..0.008 rows=1 loops=1)
Index Cond: (user_id = 13455)
-> Nested Loop (cost=0.41..104015.12 rows=3504 width=36) (actual time=0.026..3.503 rows=548 loops=1)
-> Seq Scan on categories c (cost=0.00..13.73 rows=73 width=8) (actual time=0.003..0.039 rows=73 loops=1)
-> Index Only Scan using index_topics_on_updated_at_public on topics (cost=0.41..1424.20 rows=48 width=28) (actual time=0.012..0.046 rows=8 loops=73)
Index Cond: ((updated_at >= us.first_unread_at) AND (category_id = c.id))
Filter: (NOT (COALESCE((SubPlan 1), '{}'::integer[]) && '{451,452,453}'::integer[]))
Heap Fetches: 553
SubPlan 1
-> Aggregate (cost=4.31..4.32 rows=1 width=32) (actual time=0.002..0.002 rows=1 loops=548)
-> Index Only Scan using index_topic_tags_on_topic_id_and_tag_id on topic_tags (cost=0.29..4.31 rows=1 width=4) (actual time=0.002..0.002 rows=1 loops=548)
Index Cond: (topic_id = topics.id)
Heap Fetches: 178
-> Hash (cost=97222.14..97222.14 rows=19914 width=16) (actual time=87.545..87.546 rows=42884 loops=1)
Buckets: 65536 (originally 32768) Batches: 1 (originally 1) Memory Usage: 2387kB
-> Bitmap Heap Scan on topic_users tu (cost=1217.47..97222.14 rows=19914 width=16) (actual time=14.419..78.286 rows=42884 loops=1)
Recheck Cond: (user_id = 13455)
Filter: (COALESCE(notification_level, 1) >= 2)
Rows Removed by Filter: 15839
Heap Blocks: exact=45285
-> Bitmap Index Scan on index_topic_users_on_user_id_and_topic_id (cost=0.00..1212.49 rows=59741 width=0) (actual time=6.448..6.448 rows=58723 loops=1)
Index Cond: (user_id = 13455)
SubPlan 2
-> Nested Loop Left Join (cost=0.74..46.90 rows=1 width=4) (never executed)
Join Filter: (category_users2.category_id = categories2.id)
Filter: (((category_users.id IS NULL) AND (COALESCE(category_users2.notification_level, 1) = 0)) OR (COALESCE(category_users.notification_level, 1) = 0))
-> Nested Loop Left Join (cost=0.45..32.31 rows=73 width=16) (never executed)
Join Filter: (category_users.category_id = categories.id)
-> Nested Loop Left Join (cost=0.15..18.45 rows=73 width=8) (never executed)
-> Seq Scan on categories (cost=0.00..13.73 rows=73 width=8) (never executed)
-> Memoize (cost=0.15..0.28 rows=1 width=4) (never executed)
Cache Key: categories.parent_category_id
Cache Mode: logical
-> Index Only Scan using categories_pkey on categories categories2 (cost=0.14..0.27 rows=1 width=4) (never executed)
Index Cond: (id = categories.parent_category_id)
Heap Fetches: 0
-> Materialize (cost=0.29..11.69 rows=2 width=12) (never executed)
-> Index Scan using idx_category_users_user_id_category_id on category_users (cost=0.29..11.68 rows=2 width=12) (never executed)
Index Cond: (user_id = 13455)
-> Materialize (cost=0.29..11.69 rows=2 width=8) (never executed)
-> Index Scan using idx_category_users_user_id_category_id on category_users category_users2 (cost=0.29..11.68 rows=2 width=8) (never executed)
Index Cond: (user_id = 13455)
Planning Time: 1.740 ms
Execution Time: 91.414 ms
(59 rows)
```
From the execution plan, we can see the most of the time is spent
joining about 42888 rows in the `topics` table to the `topic_users` table.
However, we know that we only have to scan through a
subset of the `topics` table because the user's last unread at is '2023-07-20 11:33:05'.
If we filter the `topics` table with `topics.updated_at >= '2023-07-20 11:33:05'`, this would only
return about 1500 rows.
From our testing in production, the PG planner is able to execute a
better query plan when we avoid the unnecessary joins on `user_stats` just to be
able to get the user's `UserStat#first_unread_at`. Instead, we can just
pass the value of `UserStat#first_unread_at` directly as a query
parameter.
```
EXPLAIN ANALYZE
SELECT
DISTINCT topics.id as topic_id,
u.id as user_id,
topics.created_at,
topics.updated_at,
topics.highest_staff_post_number AS highest_post_number,
last_read_post_number,
c.id as category_id,
c.topic_id AS category_topic_id,
tu.notification_level,
GREATEST(
CASE
WHEN COALESCE(uo.new_topic_duration_minutes, 2880) = -1 THEN u.created_at
WHEN COALESCE(uo.new_topic_duration_minutes, 2880) = -2 THEN COALESCE(
u.previous_visit_at,u.created_at
)
ELSE ('2023-07-31 03:29:45.737630'::timestamp - INTERVAL '1 MINUTE' * COALESCE(uo.new_topic_duration_minutes, 2880))
END, u.created_at, '2023-07-25 15:06:44'
) AS treat_as_new_topic_start_date
FROM topics
JOIN users u on u.id = 13455
JOIN user_options AS uo ON uo.user_id = u.id
JOIN categories c ON c.id = topics.category_id
LEFT JOIN topic_users tu ON tu.topic_id = topics.id AND tu.user_id = u.id
WHERE u.id = 13455 AND
topics.updated_at >= '2023-07-20 11:33:05' AND
topics.archetype <> 'private_message' AND
(("topics"."deleted_at" IS NULL AND (tu.last_read_post_number < topics.highest_staff_post_number) AND (COALESCE(tu.notification_level, 1) >= 2)) OR (1=0)) AND
NOT (
COALESCE((select array_agg(tag_id) from topic_tags where topic_tags.topic_id = topics.id), ARRAY[]::int[]) && ARRAY[451,452,453]
) AND
topics.deleted_at IS NULL AND
NOT (
last_read_post_number IS NULL AND
(
topics.category_id IN (SELECT "categories"."id" FROM "categories" LEFT JOIN categories categories2 ON categories2.id = categories.parent_category_id LEFT JOIN category_users ON category_users.category_id = categories.id AND category_users.user_id = 13455 LEFT JOIN category_users category_users2 ON category_users2.category_id = categories2.id AND category_users2.user_id = 13455 WHERE ((category_users.id IS NULL AND COALESCE(category_users2.notification_level, 1) = 0) OR COALESCE(category_users.notification_level, 1) = 0))
AND tu.notification_level <= 1
)
);
```
Note how the filter is now `topics.updated_at >= '2023-07-20 11:33:05'`
instead of `topics.updated_at >= us.first_unread_at`. The modified query
above generates the following execution plan.
```
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Unique (cost=5189.86..5189.88 rows=1 width=52) (actual time=4.991..5.002 rows=14 loops=1)
-> Sort (cost=5189.86..5189.86 rows=1 width=52) (actual time=4.990..4.994 rows=14 loops=1)
Sort Key: topics.id, topics.created_at, topics.updated_at, topics.highest_staff_post_number, tu.last_read_post_number, c.id, c.topic_id, tu.notification_level, (GREATEST(CASE WHEN (COALESCE(uo.new_topic_duration_minutes, 2880) = '-1'::integer) THEN u.created_at WHEN (COALESCE(uo.new_topic_duration_minutes, 2880) = '-2'::integer) THEN COALESCE(u.previous_visit_at, u.created_at) ELSE ('2023-07-31 03:29:45.73763'::timestamp without time zone - ('00:01:00'::interval * (COALESCE(uo.new_topic_duration_minutes, 2880))::double precision)) END, u.created_at, '2023-07-25 15:06:44'::timestamp without time zone))
Sort Method: quicksort Memory: 26kB
-> Nested Loop (cost=52.11..5189.85 rows=1 width=52) (actual time=0.093..4.974 rows=14 loops=1)
-> Nested Loop (cost=51.70..5181.39 rows=1 width=60) (actual time=0.084..4.931 rows=14 loops=1)
-> Nested Loop (cost=51.28..5172.94 rows=1 width=44) (actual time=0.076..4.887 rows=14 loops=1)
-> Nested Loop (cost=0.41..1698.46 rows=59 width=36) (actual time=0.029..3.537 rows=548 loops=1)
-> Seq Scan on categories c (cost=0.00..13.73 rows=73 width=8) (actual time=0.005..0.039 rows=73 loops=1)
-> Index Only Scan using index_topics_on_updated_at_public on topics (cost=0.41..23.07 rows=1 width=28) (actual time=0.012..0.047 rows=8 loops=73)
Index Cond: ((updated_at >= '2023-07-20 11:33:05'::timestamp without time zone) AND (category_id = c.id))
Filter: (NOT (COALESCE((SubPlan 1), '{}'::integer[]) && '{451,452,453}'::integer[]))
Heap Fetches: 552
SubPlan 1
-> Aggregate (cost=4.31..4.32 rows=1 width=32) (actual time=0.002..0.002 rows=1 loops=548)
-> Index Only Scan using index_topic_tags_on_topic_id_and_tag_id on topic_tags (cost=0.29..4.31 rows=1 width=4) (actual time=0.002..0.002 rows=1 loops=548)
Index Cond: (topic_id = topics.id)
Heap Fetches: 178
-> Index Scan using index_topic_users_on_user_id_and_topic_id on topic_users tu (cost=50.86..58.88 rows=1 width=16) (actual time=0.002..0.002 rows=0 loops=548)
Index Cond: ((user_id = 13455) AND (topic_id = topics.id))
Filter: ((COALESCE(notification_level, 1) >= 2) AND (last_read_post_number < topics.highest_staff_post_number) AND ((last_read_post_number IS NOT NULL) OR (NOT (hashed SubPlan 2)) OR (notification_level > 1)))
Rows Removed by Filter: 0
SubPlan 2
-> Nested Loop Left Join (cost=0.74..50.43 rows=1 width=4) (never executed)
Join Filter: (category_users2.category_id = categories2.id)
Filter: (((category_users.id IS NULL) AND (COALESCE(category_users2.notification_level, 1) = 0)) OR (COALESCE(category_users.notification_level, 1) = 0))
-> Nested Loop Left Join (cost=0.45..35.84 rows=73 width=16) (never executed)
Join Filter: (category_users.category_id = categories.id)
-> Nested Loop Left Join (cost=0.15..21.97 rows=73 width=8) (never executed)
-> Seq Scan on categories (cost=0.00..13.73 rows=73 width=8) (never executed)
-> Memoize (cost=0.15..0.61 rows=1 width=4) (never executed)
Cache Key: categories.parent_category_id
Cache Mode: logical
-> Index Only Scan using categories_pkey on categories categories2 (cost=0.14..0.60 rows=1 width=4) (never executed)
Index Cond: (id = categories.parent_category_id)
Heap Fetches: 0
-> Materialize (cost=0.29..11.69 rows=2 width=12) (never executed)
-> Index Scan using idx_category_users_user_id_category_id on category_users (cost=0.29..11.68 rows=2 width=12) (never executed)
Index Cond: (user_id = 13455)
-> Materialize (cost=0.29..11.69 rows=2 width=8) (never executed)
-> Index Scan using idx_category_users_user_id_category_id on category_users category_users2 (cost=0.29..11.68 rows=2 width=8) (never executed)
Index Cond: (user_id = 13455)
-> Index Scan using users_pkey on users u (cost=0.42..8.44 rows=1 width=20) (actual time=0.003..0.003 rows=1 loops=14)
Index Cond: (id = 13455)
-> Index Scan using index_user_options_on_user_id_and_default_calendar on user_options uo (cost=0.42..8.44 rows=1 width=8) (actual time=0.002..0.002 rows=1 loops=14)
Index Cond: (user_id = 13455)
Planning Time: 1.281 ms
Execution Time: 5.092 ms
(48 rows)
```
With the new query, PG first does an index scan using the `index_topics_on_updated_at_public` index to filter away most of the topics making the subsequent joins much cheaper. Total query time has been reduced from ~90ms to ~5ms.
This optimisation will mostly affect users with very few/recent unread topics since a large `UserStat#firsts_unread_at` value will still mean scanning through a large portion of the `topics` table.
