Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extract comments to a separate table #22295

Merged
merged 69 commits into from
Sep 6, 2024
Merged

Extract comments to a separate table #22295

merged 69 commits into from
Sep 6, 2024

Conversation

licitdev
Copy link
Member

@licitdev licitdev commented Apr 23, 2024 •
edited by rijkvanzanten
Loading

Scope

What's changed:

  • A new directus_comments table for comments

Potential Risks / Drawbacks

  • Existing users reading into comments outside of the data studio will have to update their usage

Review Notes / Questions

  • Activity and revisions now exist for tracking
  • Additional logic is added in the API to support legacy app queries to update comments
  • A migration is required in a subsequent version to migrate legacy comments in directus_activity to directus_comments
  • A legacy comment is only migrated into directus_comments when it is updated or deleted
  • The legacy app usage can be tested by building the app from main and accessing it from the dev api server in this PR

Related to #17894

Closes SER-255

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Apr 23, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 47791b9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 23 packages
Name Type
@directus/api Minor
@directus/sdk Major
@directus/system-data Minor
@directus/specs Minor
@directus/types Minor
@directus/app Minor
directus Patch
@directus/composables Patch
@directus/utils Patch
@directus/extensions-sdk Patch
@directus/extensions Patch
@directus/themes Patch
@directus/validation Patch
@directus/env Patch
@directus/extensions-registry Patch
@directus/memory Patch
@directus/pressure Patch
@directus/storage-driver-azure Patch
@directus/storage-driver-cloudinary Patch
@directus/storage-driver-gcs Patch
@directus/storage-driver-s3 Patch
@directus/storage-driver-supabase Patch
create-directus-extension Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@licitdev licitdev changed the title Extract comments to separate table Extract comments to a separate table Apr 24, 2024
@licitdev licitdev marked this pull request as ready for review April 26, 2024 08:39
@licitdev @DanielBiegler
Update loadUserPreviews typing
02c1be3 
Co-authored-by: Daniel Biegler <[email protected]>
@ComfortablyCoding ComfortablyCoding mentioned this pull request Aug 19, 2024
Open
2 tasks
@licitdev licitdev marked this pull request as ready for review August 21, 2024 16:32
@ComfortablyCoding
Copy link
Contributor

ComfortablyCoding commented Sep 3, 2024 •
edited
Loading

LGTM 🚀

Some things to note:

  1. For older comments the ID changes from a number (from revisions table) to a UUID (from comments table) on first update after API code changes. This can lead to confusion for subsequent updates if the user attempts to use the old (number) id a second time. Effect should be minimal as the app does account for this.
  2. The non-existent collection Internal Server error fix in a12b49b applies only to non admin users, for admin accounts this check is bypassed altogether and results in a Foreign Key error.

P.S. Left off approval as I was not sure if #2 was expected or not

@licitdev licitdev changed the base branch from main to retentions-p1 September 3, 2024 15:43
DanielBiegler
DanielBiegler approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@DanielBiegler DanielBiegler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the latest rounds of changes it worked for me, lets move on to the testing branch

@DanielBiegler DanielBiegler merged commit 6cb0ac1 into retentions-p1 Sep 6, 2024
4 checks passed
@DanielBiegler DanielBiegler deleted the directus-comments branch September 6, 2024 11:54
@phazonoverload phazonoverload mentioned this pull request Sep 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanneskuettner hanneskuettner hanneskuettner left review comments

@ComfortablyCoding ComfortablyCoding ComfortablyCoding left review comments

@DanielBiegler DanielBiegler DanielBiegler approved these changes

@br41nslug br41nslug br41nslug left review comments

@paescuj paescuj Awaiting requested review from paescuj

Assignees

@DanielBiegler DanielBiegler

Labels
Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@licitdev @ComfortablyCoding @hanneskuettner @br41nslug @DanielBiegler @alexchopin @paescuj @rijkvanzanten