chore(content-releases): releases migration to v5 #20259
Conversation
* chore: migrate bulkDelete to v5 * chore: change findLocales type to accept strings array * fix: docs prettier styles * chore: remove console.log
…into v5/bulk-publish-unpublish
Feranchz
added
pr: chore
Feranchz
requested review from
simotae14,
madhurisandbhor,
remidej,
Marc-Roig and
markkaylor
This comment was marked as spam.
This comment was marked as spam.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/core/content-releases/server/src/controllers/release.ts
Outdated
Show resolved
Hide resolved
packages/core/content-releases/admin/src/components/ReleasesPanel.tsx
Outdated
Show resolved
Hide resolved
packages/core/content-releases/admin/src/components/ReleasesPanel.tsx
Outdated
Show resolved
Hide resolved
packages/core/content-releases/admin/src/components/AddToReleaseAction.tsx
Outdated
Show resolved
Hide resolved
packages/core/content-releases/server/src/controllers/release-action.ts
Outdated
Show resolved
Hide resolved
|
|
||
| type ReleaseWithPopulatedActions = Release & { actions: { count: number } }; | ||
|
|
||
| const releaseController = { | ||
| async findMany(ctx: Koa.Context) { | ||
| async findByDocumentAttached(ctx: Koa.Context) { |
There was a problem hiding this comment.
could you add some comment or explanation of what's the responsability of this controller? 🤔
After 2 weeks of being of, I forgot a bit the logic, and I don't quite see what findByDocumentAttached means.
Maybe what confuses me is the "Attached" word
There was a problem hiding this comment.
Also, it feels off that the endpoint is called findByDocumentAttached, but you can send a parameter to find by documents not attached 🤔
There was a problem hiding this comment.
It's true, but do you think it's worth to create a different endpoint & controller to split the 2 cases? 🤔
| const relatedReleases = await releaseService.findMany({ | ||
| where: { | ||
| releasedAt: null, | ||
| releasedAt: { | ||
| $null: true, | ||
| }, | ||
| actions: { | ||
| target_type: contentTypeUid, | ||
| target_id: entryId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const releases = await releaseService.findMany({ | ||
| where: { | ||
| $or: [ | ||
| { | ||
| id: { | ||
| $notIn: relatedReleases.map((release: any) => release.id), | ||
| }, | ||
| }, | ||
| { | ||
| actions: null, | ||
| }, | ||
| ], | ||
| releasedAt: { | ||
| $null: true, | ||
| }, |
There was a problem hiding this comment.
I believe these queries could be inside the service itself.
releases.findEntryReleases({id, populate...})
releases.findNotInEntryReleases({id, ...))
You could also reduce logic here if you do something like:
releases.findDocumentReleases({documentId, locale, populate...})
releases.findNotInDocumentReleases({documentId, locale, ...))
There was a problem hiding this comment.
I'm not 100% sure about this but we can discuss it when you're back
It's true that we reduce the controller complexity but we increase the service complexity and we need to maintain and look up for breaking changes in the service but not in the controller. What I mean is: Users can use the releases service in plugins or other places so in my opinion service's API should be as simple as possible and all these functions are just different way of using the findMany that already exists in the service
|
Don't know if it's something already fixed in the v5 branch , but it's a bit difficult for me to test strapi using multiple users atm. It's not possible for me to refresh the page when I have two tabs (one in incognito) with two different users. |
… type to handle relations
What does it do?
Migrates and apply new changes on the Content Releases plugin for v5.
Work to do