* FIX: Division by zero error on WebHookEventsDailyAggregate
* DEV: Update implementation of WebHookEventsDailyAggregate to handle division by zero error
I am changing many of these to notes or resolving them as is,
most of these I have not actively worked on in years so someone
else can work on them when we get to these areas again.
This commit continues work laid out by ffec8163b0 for the admin config page for the /about page. The last commit set up the user interface, and this one sets up all the wiring needed to make the input fields and save buttons actually work.
Internal topic: t/128544.
While creating a new category if the user didn't specify a value for `minimum_required_tags` input but clicked it then it returned the "PG::NotNullViolation: null value in column 'minimum_required_tags'" error.
When bad data is provided in the URI for redirecting to a category,
Rails raises an `ActionController::Redirecting::UnsafeRedirectError`
error, leading to a 500 error.
This patch catches the exception to render a 404 instead.
Currently redirecting to an external URL through a permalink doesn’t
work because Rails raises a
`ActionController::Redirecting::UnsafeRedirectError` error.
This wasn’t the case before we upgraded to Rails 7.0.
This patch fixes the issue by using `allow_other_host: true` on the
redirect.
If, for whatever reasons, the user's locale is "blank" and an admin is accepting their group membership request, there will be an error because we're generating posts with the locale of recipient.
In order to fix this, we now use the `user.effective_locale` which takes care of multiple things, including returning the default locale when the user's locale is blank.
Internal ref - t/132347
When using the full page search and filtering down to a specific topic, the sort order was overwritten to by by "post_number".
This was confusing because we allow different type of sort order in the full search page.
This fixes it by only sorting by post_number when there's no "global" sort order defined.
Since the "new topic map" uses the search endpoint behind the scene, this also fixes the "most likes" popup.
Context - https://meta.discourse.org/t/searching-order-seems-to-be-broken-when-searching-in-topic/312303
* DEV: add db migration to filter out invalid csp script source values
* DEV: insert UserHistory row during data migration to track old value for content_security_policy_script_src site setting
When visiting a user profile, and then opening the search, there's an option to filter down by posts made by that user.
When clicking that option, it used to pre-fill the "search bar" with "@<username>" to filter down the search.
This restore this behaviour and add a system spec to ensure it doesn't regress.
Context - https://meta.discourse.org/t/in-posts-by-search-option-does-not-work-when-clicked/312916
We want to allow admins to make new required fields apply to existing users. In order for this to work we need to have a way to make those users fill up the fields on their next page load. This is very similar to how adding a 2FA requirement post-fact works. Users will be redirected to a page where they can fill up the remaining required fields, and until they do that they won't be able to do anything else.
Followup 96a0781bc1
When sending emails where secure uploads is enabled
and secure_uploads_allow_embed_images_in_emails is
true, we attach the images to the email, and we
do some munging with the final email so the structure
of the MIME parts looks like this:
```
multipart/mixed
multipart/alternative
text/plain
text/html
image/jpeg
image/jpeg
image/png
```
However, we were not specifying the `boundary` of the
`multipart/mixed` main content-type of the email, so
sometimes the email would come through appearing to
have an empty body with the entire thing attached as
one attachment, and some mail parsers considered the
entire email as the "epilogue" and/or "preamble".
This commit fixes the issue by specifying the boundary
in the content-type header per https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html
Adds a checkbox to filter untranslated text strings in the admin UI, behind a hidden and default `false` site setting `admin_allow_filter_untranslated_text`.
Followup to 0e1102b332
Minor followup, makes the condition check against the
boolean val, see the difference here:
```ruby
!SiteSetting.enforce_second_factor_on_external_auth && "true"
=> "true"
```
vs:
```ruby
!SiteSetting.enforce_second_factor_on_external_auth && "true" == "true"
=> true
```
* Removed the link from the title, so the settings can only be accessed via the settings button on the right
* Added an icon to the "Learn more" link to indicate that it opens a new window
* Made various styling adjustments
* DEV: Upgrade Rails to 7.1
* FIX: Remove references to `Rails.logger.chained`
`Rails.logger.chained` was provided by Logster before Rails 7.1
introduced their broadcast logger. Now all the loggers are added to
`Rails.logger.broadcasts`.
Some code in our initializers was still using `chained` instead of
`broadcasts`.
* DEV: Make parameters optional to all FakeLogger methods
* FIX: Set `override_level` on Logster loggers (#27519)
A followup to f595d599dd
* FIX: Don’t duplicate Rack response
---------
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
In some instances, the `modifications` of `tags` hasn't been properly serialized as a Ruby array but rather as a string (I've seen `""`, `"[]"`, and `"[\"\"]"`).
This generates an error when we try to `filter_tags` and remove `hidden_tags` (which is an array) from `tags` which might be a string.
Internal ref - t/131126
I wasn't able to figure out the root cause of this so I reverted the behavior that was introduced ~6 years ago in f2c060bdf2
This ensures that the theme id is resolved as early as possible in the
request cycle. This is necessary for the custom homepage to skip
preloading the wrong data.
Previously filter route was not setting topic list, this meant that
keyboard navigation using "G" "J" was not functioning.
This amends it by ensuring the list is set after looking up the model.
* DEV: Upgrade Rails to 7.1
* FIX: Remove references to `Rails.logger.chained`
`Rails.logger.chained` was provided by Logster before Rails 7.1
introduced their broadcast logger. Now all the loggers are added to
`Rails.logger.broadcasts`.
Some code in our initializers was still using `chained` instead of
`broadcasts`.
* DEV: Make parameters optional to all FakeLogger methods
* FIX: Set `override_level` on Logster loggers (#27519)
A followup to f595d599dd
* FIX: Don’t duplicate Rack response
---------
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
In this PR we introduced `enforce_second_factor_on_external_auth` setting https://github.com/discourse/discourse/pull/27506
When it is set to false and the user is authenticated via OAuth, then we should not enforce the 2fa configuration.
We recently fixed a problem where secure upload images weren't re-attached when sending the activity summary e-mail.
This fix contained a bug that would lead to n copies of the e-mail body being included, n being the number of duplicates. This is because #fix_parts_after_attachments! was called once per attachment, and adding more parts to the multipart e-mail.
This PR fixes that by:
Adding a failing test case for the above.
Moving the looping over multiple posts into #fix_parts_after_attachments! itself.
When a post is cooked the links are extracted and `TopicLink` instances
are created for each of them. These links are used in various places,
including the topic view, user summary page, etc.
In previous commit 48e5d1a, hotlinked images from Oneboxes have been
ignored from the texts, but hotlinked images turned into Lightboxes
were still extracted.
The test that checks that securely uploaded images are re-attached to the digest e-mail wasn't rendering the actual digest e-mail template. This change fixes that.
Many site settings can be distructive or have huge side-effects
for a site that the admin may not be aware of when changing it.
This commit introduces a `requires_confirmation` attribute that
can be added to any site setting. When it is true, a confirmation
dialog will open if that setting is changed in the admin UI,
optionally with a custom message that is defined in client.en.yml.
If the admin does not confirm, we reset the setting to its previous
clean value and do not save the new value.
Some tooling may rely on an unsafe-none cross origin opener policy to work. This change adds a hidden site setting that can be used to list referrers where we add this header instead of the default one configured in cross_origin_opener_policy_header.
For Topic Embeds, we would prefer <article> to be the main article in a topic, rather than a table cell <td> with potentially a lot of data. However, in an example URL like here, the table cell (the very large code snippet) is seen as the Topic Embed's article due to the determined content weight by the Readability library we use.
In the newly released 0.7.1 cantino/ruby-readability#94, the library has a new option to exclude the library's default <td> element into content weighting. This is more in line with the original library where they only weighted <p>. So this PR excludes the td, as seen in the tests, to allow the actual article to be seen as the article. This PR also adds the details tag into the allow-list.
Followup 6b872c4c53
Even though we were showing a validation error for a reject
reason that was too long, we were still sending an email and
doing other operations on the user which we are rejecting.
This commit fixes this by validating the reviewable model
before attempting to do anything else after the reason is set.
A new admin setting called `enforce_second_factor_on_external_auth`. It allows users to authenticate using external providers even when 2FA is forced with `enforce_second_factor` site setting.
* Revert "FIX: Set `override_level` on Logster loggers (#27519)"
This reverts commit c1b0488c54.
* Revert "DEV: Make parameters optional to all FakeLogger methods"
This reverts commit 3318dad7b4.
* Revert "FIX: Remove references to `Rails.logger.chained`"
This reverts commit f595d599dd.
* Revert "DEV: Upgrade Rails to 7.1"
This reverts commit 081b00391e.
`Rails.logger.chained` was provided by Logster before Rails 7.1
introduced their broadcast logger. Now all the loggers are added to
`Rails.logger.broadcasts`.
Some code in our initializers was still using `chained` instead of
`broadcasts`.
Currently when a cache entry is corrupt, we log the event without doing
anything else. It means the cache is still corrupt, and the proper value
isn’t computed again.
Normally, it’s very rare the cache becomes corrupt, but it can happen
when upgrading Rails for example and the cache format changes. This is
normally handled automatically by Rails but since we’re using a custom
cache class, we have to do it ourselves.
This patch takes the same approach the Rails team did, when a cache
entry is corrupt, we treat it as a miss, recomputing the proper value
and caching it in the new format.
Wasn't quite handling the cases where a closing bracket `]` was used in the value of one of the attributes.
```markdown
[chat quote=user channel="[broken]"]
```
Would not be correctly parsed because we would _greedily_ use the first `]` as the end of the tag even though it might be a valid character when inside proper quotes.
c39a4de139/app/assets/javascripts/discourse-markdown-it/src/features/bbcode-block.js (L62)
Re-wrote the `parseBBCodeTag` to properly handle the following cases
- A closing tag (aka `[/name]`) which are easy since they don't have any attributes
- An old `[quote=...]` format we used that doesn't uses quotes but still has various attributes of the form `key:value`
- All three valid BBCode opening tag formats we support
- `[name]` without any attributes
- `[name=foo]` with a default value
- `[name foo=bar]` with some attributes
Ended up having to fix/rewrite the few bbcode rules that were using the `parseBBCodeTag` function, namely `d-wrap` and `discourse-local-dates`.
While working on this, I think I also found a way to get rid the of shims we had in place so that plugins could use the `parseBBCodeTag` function.
Reference - https://meta.discourse.org/t/having-a-right-bracket-in-a-channel-name-breaks-all-quotes-from-that-channel/308439
* Load search results in displayed order so that when more categories are loaded on scroll, they appear at the end,
* Limit the number of subcategories that are shown per category and display 'show more' links,
This commit adds ability to fetch a subset of site settings from the `/admin/site_settings` endpoint so that it can be used in all places where the client app needs access to a subset of the site settings.
Additionally, this commit also introduces a new service class called `UpdateSiteSetting` that encapsulates all the logic that surrounds updating a site setting so that it can be used to update site setting(s) anywhere in the backend. This service comes in handy with, for example, the controller for the flags admin config area which may need to update some site settings related to flags.
Internal topic: t/130713.
This commits introduces the `sidekiq_report_long_running_jobs_minutes`
global setting which allows a site administrator to log a warning in the
Rails log when a Sidekiq job has been running for too long.
The warning is logged with the backtrace of the thread that is
processing the Sidekiq job to make it easier to figure out what a
sidekiq job is stuck on.
When we turn on settings automatically for customers,
we sometimes use `.set_and_log` which will make a staff
action log for the site setting change. This is fine, but
there is no context for customers.
This change allows setting a message with `.set_and_log`, which
will be stored in the `details` column of the staff action log
created, which will show up on `/admin/logs/staff_action_logs`
---------
Co-authored-by: Kelv <kelv@discourse.org>
In #26642 we introduced a change that re-attaches securely uploaded images in the digest e-mail. However, this change assumed that the type argument to the Email::Sender constructor would be a symbol, but when it is coming from the UserEmail job it is a string. This PR fixes that.
This introduces the syntax of
`category:a,b,c` which will search across multiple categories.
Previously there was no way to allow search across a wide selection of
categories.
After working on the Webhook events filter by Status, I noticed that the 'Delivered' and 'Failed' options do not take the status param when loading more than fifty Webhook events. It causes to load all Webhook events regardless of its status after the first load.
This PR is adding webhook events status for the filter to the param when loading more than fifty Webhook events.
This commit tries another work around for the `Socket::ResolutionError: getaddrinfo: Temporary failure in name resolution`
error we are seeing on CI.
The problem with the previous workaround is that `Capybara.using_session` will attempt to resolve `localhost`
before yielding the block which means our retry code is not hit.
This problem may be related to https://bugs.ruby-lang.org/issues/20172
which hints at us potentially not being able to spin up threads on CI so
I'm adding a debugging statement when stuff fails.
decorator-transforms (https://github.com/ef4/decorator-transforms) is a modern replacement for babel's plugin-proposal-decorators. It provides a decorator implementation using modern browser features, without needing to enable babel's full suite of class feature transformations. This improves the developer experience and performance.
In local testing with Google's 'tachometer' tool, this reduces Discourse's 'init-to-render' time by around 3-4% (230ms -> 222ms).
It reduces our initial gzip'd JS payloads by 3.2% (2.43MB -> 2.35MB), or 7.5% (14.5MB -> 13.4MB) uncompressed.
This was previously reverted in 97847f6. This version includes a babel transformation which works around the bug in Safari <= 15.
For Cloudflare compatibility issues, check https://meta.discourse.org/t/311390
Not all HTML elements are converted into Markdown. Some are kept as HTML.
Without this fix XML/HTML entities that are formatted as text instead of code are swallowed by Discourse.
This also fixes quotes in the `title` attribute of the `<abbr>` tag.
Previously `HtmlToMarkdown` always converted HTML tables into Markdown tables. That lead to some badly formatted Markdown tables, e.g. when the table contained `rowspan` or `colspan`. This solves the issue by using very basic HTML tables in those cases.
When running ` rspec spec/services/external_upload_manager_spec.rb`
in the development environment, tests were failing with the following
error:
```
NameError:
uninitialized constant FakeS3::Aws
```
This commit introduces a hidden `s3_inventory_bucket` site setting which
replaces the `enable_s3_inventory` and `s3_configure_inventory_policy`
site setting.
The reason `enable_s3_inventory` and `s3_configure_inventory_policy`
site settings are removed is because this feature has technically been
broken since it was introduced. When the `enable_s3_inventory` feature
is turned on, the app will because configure a daily inventory policy for the
`s3_upload_bucket` bucket and store the inventories under a prefix in
the bucket. The problem here is that once the inventories are created,
there is nothing cleaning up all these inventories so whoever that has
enabled this feature would have been paying the cost of storing a whole
bunch of inventory files which are never used. Given that we have not
received any complains about inventory files inflating S3 storage costs,
we think that it is very likely that this feature is no longer being
used and we are looking to drop support for this feature in the not too
distance future.
For now, we will still support a hidden `s3_inventory_bucket` site
setting which site administrators can configure via the
`DISCOURSE_S3_INVENTORY_BUCKET` env.
This is a follow up to 9ff0805a1d. We
noticed that `localhost` can fail to resolve in other spots of the app
and not just in selenium-webdriver.
From the failing tests we have seen, the `getaddrinfo: Temporary failure in name resolution` error is only
seen from within the `Capybara.using_session` block. This commit aims to
ensure that `localhost` can be resolve after the new session is started.
* FEATURE: Add Filter for Webhook Events by Status
* Fixing multiple issues
* Lint
* Fixing multiple issues
* Change the range of the status for webhook events
We want to get rid of the old topic bulk actions modal
and use the new dropdown (currently gated behind
experimental_topic_bulk_actions_enabled_groups). To do
this we need to use the new dropdown in all places in the
UI.
This commit changes the full page search UI to use the new
topic bulk actions dropdown if experimental_topic_bulk_actions_enabled_groups
is enabled, and makes some minor refactors to make this work.
Also add a spec for both the old and new functionality.
This commit fixes a problem where the user will not be able to reset
their password when they only have security keys and backup codes
configured.
This commit also makes the following changes/fixes:
1. Splits password reset system tests to
`spec/system/forgot_password_spec.rb` instead of missing the system
tests in `spec/system/login_spec.rb` which is mainly used to test
the login flow.
2. Fixes a UX issue where the `Use backup codes` or `Use authenticator
app` text is shown on the reset password form when the user does
not have either backup codes or an authenticator app configured.
The mistake was made when flags were moved to the database. The `notify_moderators` (something else) flag should be the last position on the list.
This commit contains 3 changes:
- update fixtures order;
- remove position and enable from fixtures (they can be overridden by admin and we don't want seed to restore them);
- migration to fix data if the order was not changed by admin.
This commit updates the `UserPasswordExpirer.expire_user_password`
method to update `UserPassword#password_expired_at` when an existing
`UserPassword` record exists with the same `password_salt`,
`password_hash` and `password_algorithm`. This is to prevent the unique
validation error on `UserPassword#user_id` and
`UserPassword#password_hash` from being raised when the method is called
twice for a user that has not changed its password.
This commit includes various UX improvements to the reset
password page:
* Introduce a `hide-application-header-buttons` helper to do the following:
* Hide Sign Up and Log In buttons, they are not necessary on this flow
* Hide the sidebar, it is a distraction on this flow
* Improve messaging when a 2FA confirmation is required first
* Improve display of server-side ActiveRecord model validation errors
in password form, e.g. instead of "is the same as your current password"
we do "The password is the same as your current password"
* Move password tip to next line below input and move caps lock hint
inline with Show/Hide password toggle
* Add system specs for 2FA flow on reset password page
* Fixes a computed property conflict issue on the password reset
page when toggling 2FA methods
Continued work on moderate flags UI.
In this PR admins are allowed to change the order of flags. The notify user flag is always on top but all other flags can be moved.
