refs https://github.com/TryGhost/Team/issues/2503
This is in the MentionController atm as it's considered a presentation
concern. We might want to consider moving this into the MentionsAPI in
future so that we can simplify the controller and even remove it
completely in favour of putting the data-mapping in the endpoint definition.
fixes https://github.com/TryGhost/Team/issues/481
This change fixes an issue when multiple images with the same name are
uploaded in parallel. The current system does not guarantee that the
original filename is stored under NAME+`_o`, because the upload for the
original file and the resized file are happening in parallel.
Solution:
- Wait for the storage of the resized image (= the image without the _o
suffix) before storing the original file.
- When that is stored, use the generated file name of the stored image
to generate the filename with the _o suffix. This way, it will always
match and we don't risk both files to have a different number suffix.
We'll also set the `targetDir` argument when saving the file, to avoid
storing the original file in a different directory (when uploading a
file around midnight both files could be stored in 2023/01 and 2023/02).
Some extra optimisations needed with this fix:
- Previously when uploading image.jpg, while it already exists, it would
store two filenames on e.g., `image-3.jpg` and `image_o-3.jpg`. Note the
weird positioning of `_o`. This probably caused bugs when uploading
files named `image-3.jpg`, which would store the original in
`image-3_o.jpg`, but this original would never be used by the
handle-image-sizes middleware (it would look for `image_o-3.jpg`). This
fix would solve this weird naming issue, and make it more consistent.
But we need to make sure our middlewares (including handle-image-sizes)
will be able to handle both file locations to remain compatible with the
old format. This isn't additional work, because it would fix the old bug
too.
- Prevent uploading files that end with `_o`, e.g. by automatically
stripping that suffix from uploaded files. To prevent collisions.
Advantage(s):
- We keep the original file name, which is better for SEO.
- No changes required to the storage adapters.
Downside(s):
- The storage of both files will nog happen parallel any longer. But I
expect the performance implications to be minimal.
- Changes to the routing: normalize middleware is removed
no issue
There are a couple of issues with resetting the Ghost instance between
E2E test files:
These issues came to the surface because of new tests written in
https://github.com/TryGhost/Ghost/pull/16117
**1. configUtils.restore does not work correctly**
`config.reset()` is a callback based method. On top of that, it doesn't
really work reliably (https://github.com/indexzero/nconf/issues/93)
What kinda happens, is that you first call `config.reset` but
immediately after you correcty reset the config using the `config.set`
calls afterwards. But since `config.reset` is async, that reset will
happen after all those sets, and the end result is that it isn't reset
correctly.
This mainly caused issues in the new updated images tests, which were
updating the config `imageOptimization.contentImageSizes`, which is a
deeply nested config value. Maybe some references to objects are reused
in nconf that cause this issue?
Wrapping `config.reset()` in a promise does fix the issue.
**2. Adapters cache not reset between tests**
At the start of each test, we set `paths:contentPath` to a nice new
temporary directory. But if a previous test already requests a
localStorage adapter, that adapter would have been created and in the
constructor `paths:contentPath` would have been passed. That same
instance will be reused in the next test run. So it won't read the new
config again. To fix this, we need to reset the adapter instances
between E2E tests.
How was this visible? Test uploads were stored in the actual git
repository, and not in a temporary directory. When writing the new image
upload tests, this also resulted in unreliable test runs because some
image names were already taken (from previous test runs).
**3. Old 2E2 test Ghost server not stopped**
Sometimes we still need access to the frontend test server using
`getAgentsWithFrontend`. But that does start a new Ghost server which is
actually listening for HTTP traffic. This could result in a fatal error
in tests because the port is already in use. The issue is that old E2E
tests also start a HTTP server, but they don't stop the server. When you
used the old `startGhost` util, it would check if a server was already
running and stop it first. The new `getAgentsWithFrontend` now also has
the same functionality to fix that issue.
refs https://github.com/TryGhost/Team/issues/2419
We use a job queue to ensure that webmentions can be processed outside of
the request/response cycle, but still finish executing if the processed is closed.
With this we're able to update the e2e tests to await the processing of the mention
rather than sleepign for arbitrary lengths of time, and we've reintroduced the tests
removed previously
- aa14207b69
- 48e9393159
closes https://github.com/TryGhost/Team/issues/2429
- sends email notifications to staff users when their site receives a Webmention.
- currently behind a flag, that can be toggled in the labs settings.
This test is failing because the `sleep` isn't long enough. Removing this test
until we've refactored to use the jobs service, at which point we can remove the
sleep and wait for the job to be complete.
fixes https://github.com/TryGhost/Team/issues/2432
Adds outbound_link_tagging setting (enabled by default and behind
feature flag). If the feature flag is enabled, and the setting is
disabled, we won't add ?ref to links in emails.
This includes new E2E tests for email click tracking, which were also
extended to check outbound link tagging (for both MEGA and the new email
stability flow).
Also fixes a test fixture for the comments_enabled setting.
fixes https://github.com/TryGhost/Team/issues/2461
- Ignores 'edited' links when there is only one second differences.
- Make sure we don't set updatedAt when linking a post to a redirect
refs acf0baa8c7
Due to the bump in express-test, we now handle string bodies 'properly'. So they now pass all the Express middlewares. In the past this failing test did not really pass by the bodyParser.raw middleware,
so the content-type check on the `bodyParser.raw({type: 'application/json'})` middleware was not executed. Now it is, and the test fails because the content-type header was not set to application/json.
refs https://github.com/TryGhost/Team/issues/2400
- we've deemed it useful to start to return `Content-Version` for all
API requests, because it becomes useful to know which version of Ghost
a response has come from in logs
- this should also help us detect Admin<->Ghost API mismatches, which
was the cause of a bug recently (ref'd issue)
refs https://github.com/TryGhost/Toolbox/issues/499
- Outgoing emails have been a weak point of Ghost's stability recently. The concept of "emailMockReceiver" similarly to "webhookMockReceiver", allows to test side-effects like outgoing emails.
- This is a first iteration which should lay groundwork for testing all outgoing emails in the future
- The change adds a new concept of "email mock receiver" which is very similar to how the "webhook mock receiver" works. The email mock receiver exposes two methods to record and verify snapshots:
- matchHTMLSnapshot - records and verifies only the HTML content of the outgoint email
- matchMetadataSnapshot - records and verifies all the non-HTML properties sent along an email content, e.g.: to address, plaintext, subject, etc.
- What's missing is matching content based on dynamic content like dates, links with JWT tokens, etc.
This introduces the new suppressions feature which will automatically
unsubscribe members from newsletters when their email is added to the
suppression list in Mailgun, this is usually due to emails either
permanently bouncing to the address, or the member making a spam
complaint.
Both Members and Admins are able to see that the email has been added to
the list, and Members are be able to request their email be removed from
the list via Portal.
Overall this feature should improve delivery rates of newsletters and
improve the rating of the domain you're sending from.
closes https://github.com/TryGhost/Team/issues/2011
- Gives publishers the ability to filter members based on which offer they used (redeemed) when they subscribed for a paid membership.
- On the offers page, the redemption count number links to a the members page with the filter already applied making it easy to have insight on which members used the offer / coupon.
refs https://github.com/TryGhost/Team/issues/2393
- During boot and loading the active theme, we now cache the result of
the gscan validation. Cache configuration can happen in
`adapters.cache.gscan`
- We now also return non-fatal errors when activating or adding a theme.
- When the `themeErrorsNotification` feature flag is on, we fetch the
active theme (which includes the validation information) when loading
admin
- If the currently active theme has errors, we show an error
notification that can open the error modal
- Added a new endpoint: `/ghost/api/admin/themes/active/` that returns
the result of the last gscan validation of the active theme. If no cache
is available, it will run a new gscan validation.
- Added new permissions for the active action/endpoint (author, editor,
administrator)
The email_recipient fixtures were using duplicate and mismatched email addresses
rather than having them correctly map to the Members, which is required for testing
email suppressions.
no issue
With the increased usage of DomainEvents, it gets harder to build
reliable tests without having to resort to timeouts. This utility method
allows us to wait for all events to be processed before continuing with
the test.
This change should speed up tests and make them more reliable.
It only adds extra code when running tests and shouldn't impact
production.
fixes https://github.com/TryGhost/Team/issues/2366
refs https://ghost.slack.com/archives/C02G9E68C/p1670232405014209
Probem described in issue.
In the old MEGA flow:
- The `email_verification_required` check is now repeated inside the job
In the new email service flow:
- The `email_verification_required` is now checked (didn't happen
before)
- When generating the email batch recipients, we only include members
that were created before the email was created. That way it is
impossible to avoid limit checks by inserting new members between
creating an email and sending an email.
- We don't need to repeat the check inside the job because of the above
changes
Improved handling of large imports:
- When checking `email_verification_required`, we now also check if the
import threshold is reached (a new method is introduced in
vertificationTrigger specifically for this usage). If it is, we start
the verification progress. This is required for long running imports
that only check the verification threshold at the very end.
- This change increases the concurrency of fastq to 3 (refs
https://ghost.slack.com/archives/C02G9E68C/p1670232405014209). So when
running a long import, it is now possible to send emails without having
to wait for the import. Above change makes sure it is not possible to
get around the verification limits.
Refactoring:
- Removed the need to use `updateVerificationTrigger` by making
thresholds getters instead of fixed variables.
- Improved awaiting of members import job in regression test
fixes https://github.com/TryGhost/Team/issues/1996
**Issue**
Our Magic links are valid for 24 hours. After first usage, the token
lives for a further 10 minutes, so that in the case of email servers or
clients that "visit" links, the token can still be used.
The implementation of the 10 minute window uses setTimeout, meaning if
the process is interrupted, the 10 minute window is ignored completely,
and the token will continue to live for the remainder of it's 24 hour
validity period. To prevent that, the tokens are cleared on boot at the
moment.
**Solution**
To remove the boot clearing logic, we need to make sure the tokens are
only valid for 10 minutes after first use even during restarts.
This commit adds 3 new fields to the SingleUseToken model:
- updated_at: for storing the last time the token was changed/used). Not
really used atm.
- first_used_at: for storing the first time the token was used
- used_count: for storing the number of times the token has been used
Using these fields:
- A token can only be used 3 times
- A token is only valid for 10 minutes after first use, even if the
server restarts in between
- A token is only valid for 24 hours after creation (not changed)
We now also delete expired tokens in a separate job instead of on boot /
in a timeout.
fixes https://github.com/TryGhost/Team/issues/2386
**Issue:**
- When trying to import a member that already exists, and has
'subscribed' set to 'true' in the CSV, the newsletters the member is
subscribed to are reset to the default newsletters.
- When ediging a member with the API and setting `subscribed` to true,
the same happens.
**Cause:**
A faulty check for the `status` property of a newsletter.
Fixed and added a new E2E test.
refs https://github.com/TryGhost/Toolbox/issues/476
- The email verification trigger and host settings related bugs have been a cause of bugs in past releases. The admin client verification source did not have any test coverage in the past.
- The members test suite size is getting out of hand. This test is quite verbose, because of the state it's trying to check.
- In the future we should consider splitting up Member API (and probably other) test suites into smaller pieces.
no issue
- The sleep method has been used in 8 modules reimplementing the same thing over and over again. It's usually a sign of async event processing outside of the request/response loop. It's good to have a single point of implementation for a "hack" like this, so we could track it easier and address the even processing delay in a more optimal way centrally if it ever becomes a bottleneck
fixes https://github.com/TryGhost/Team/issues/2346
- Adds email batch browse endpoint
- Adds email recipient failures browse endpoint
- Adds new fixtures and E2E tests for the new API
- Added support for snapshot tests to have 'nullable' types.
refs https://github.com/TryGhost/Team/issues/2339
- Includes a new pattern in the job manager that allows us to properly
await jobs.
- Added new convenience mocking methods to stub settings
- Tests the main flows for bulk sending:
- Sending in multiple batches
- Sending to multiple segments
- Handling a failed batch and retrying that batch
- Fixes bug in batch generation (ordering not working)
In a different PR I'll add more detailed tests.
closes https://github.com/TryGhost/Team/issues/2324
- It seemed like the "limit" query parameter did not work properly returning multiple entries from the endpoint. In reality the whole query string was ignored because of an error in the "filter" part of the query ^_^
closesTryGhost/Team#2313
- Added Sent event to Post analytics and Members feed. Now post can be
Sent or Received or Bounced.
- Excluded Delivered event from Sent filter on backend.
refs https://github.com/TryGhost/Ghost/security/advisories/GHSA-9gh8-wp53-ccc6
refs https://github.com/TryGhost/Toolbox/issues/465
- Bookshelf relations allows us to edit relational records by default, which was used liberally in the codebase.
- Not having a clear track record of editable relations left the model layer prone to triggering unwanted nested saves and created a vulnerability where members were able to edit newsletter settings.
- With explicit editable relations it's easier to keep track of relations having editable access to related records. Makes the relational data modification pattern safer to use too.
- Anyone running 5.x should update to 5.24.1
Credits: Dave McDaniel and other members of [Cisco Talos](https://talosintelligence.com/vulnerability_reports)
fixes https://github.com/TryGhost/Team/issues/2284
New batch sending flow (still WIP). Logs the sent emails instead of actually sending them. Unit tests are coming in later commits.
refs https://github.com/TryGhost/Team/issues/2280
We are moving away from storing html and plaintext on email and instead will store the email data in source and source_type columns which allows us to store the email in other formats like mobiledoc and lexical. Storing in those formats allows greater flexibility for later html generation
- adds new `source` column that stores `mobiledoc`/`lexical`/`html` data for a newsletter
- adds new `source_type` column that stores one of `mobiledoc`/`lexical`/`html` to identify type of source
fixes https://github.com/TryGhost/Team/issues/2282
Added a new email service package that is used when the email stability
flag is enabled. Currently not yet implemented so will throw an error
for all entry points (if flag enabled).
Removed usage of `labs.isSet.bind` across the code, because that breaks
the stubbing of labs by `mockManager.mockLabsEnabled` and
`mockManager.mockLabsDisabled`. `flag => labs.isSet(flag)` should be
used instead.
All email depending tests now disable the `emailStability` feature flag
to keep the tests passing + make sure we still run all the tests for the
old flow while the email stability package is being built.
refs https://github.com/TryGhost/Team/issues/2268
The approach of using the service to lead email suppression data as
opposed to bookshelf relations allows us to wire things up without
having implemented the database. The getBulkSuppressionData allows us to
do this without much of a DB performance hit.
closes https://github.com/TryGhost/Team/issues/2126
- Cleaned up the following GA flags: `newsletterPaywall`, `freeTrial`, `compExpiring`, `searchHelper`, `emailAlerts`, `fixNewsletterLinks`.
