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.

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

fix(content-manager): history service #20185

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

jhoward1994
Copy link
Contributor

What does it do?

Describe the technical changes you did.

Why is it needed?

Describe the issue you are solving.

How to test it?

Provide information about the environment and the path to verify the behaviour.

Related issue(s)/PR(s)

Let us know if this is related to any issue/pull request

@jhoward1994 jhoward1994 added the flag: don't merge This PR should not be merged at the moment label Apr 23, 2024
@@ -167,7 +167,12 @@ const createHistoryService = ({ strapi }: { strapi: Core.Strapi }) => {
: { documentId: context.params.documentId, locale: context.params?.locale };

const defaultLocale = await getDefaultLocale();
const locale = documentContext.locale || defaultLocale;

// TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @markkaylor

I was having trouble with content history during the bulk locale publishing stuff I'm working on. This bit of code would cause an error to be thrown from

`Invalid locale param ${params.locale} provided. Document locales must be strings.`

I'm not 100% what the best solution is here. I think the problem is that now publishing can accept multiple locales, history will intercept those calls and try and proceed with the logic below using an array of locales from the params. docService.findOne then throws as this is an invalid param.

What do you think the best idea here is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can just check if it's an array and skip it for now 🤷‍♂️ ? Then we can circle back to adjust the logic here to handle all bulk actions. What do you think? cc @remidej

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I agree with you @markkaylor. this is a place where we need to eventually write proper tests, and we should probably be the ones to write it, so I think opting out for now in this branch should be okay

Copy link

vercel bot commented Apr 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 26, 2024 1:40pm

@jhoward1994
Copy link
Contributor Author

@remidej @markkaylor

Is it okay with you both if I merge this into our feature branch and you could track this TODO in the content studio team?

I need this change to test the whole feature from the FE

@jhoward1994 jhoward1994 marked this pull request as ready for review April 26, 2024 12:54
@jhoward1994 jhoward1994 removed the flag: don't merge This PR should not be merged at the moment label Apr 26, 2024
Comment on lines 171 to 181
let locale = defaultLocale;
if (documentContext.locale) {
if (Array.isArray(documentContext.locale)) {
// TODO calls picked from the middleware could contain an array of
// locales. This is incompatible with our call to findOne below.

return next();
}

locale = documentContext.locale;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we avoid creating a mutable variable for locale? Wouldn't this be enough?

Suggested change
let locale = defaultLocale;
if (documentContext.locale) {
if (Array.isArray(documentContext.locale)) {
// TODO calls picked from the middleware could contain an array of
// locales. This is incompatible with our call to findOne below.
return next();
}
locale = documentContext.locale;
}
if (Array.isArray(documentContext.locale)) {
// TODO calls picked from the middleware could contain an array of
// locales. This is incompatible with our call to findOne below.
return next();
}
const locale = documentContext.locale || defaultLocale;

@jhoward1994 jhoward1994 merged commit ec216aa into feat/be-bulk-locale-publish Apr 26, 2024
8 of 10 checks passed
@jhoward1994 jhoward1994 deleted the fix/history-bulk-locale branch April 26, 2024 13:34
jhoward1994 added a commit that referenced this pull request May 3, 2024
* feat(core): document service (un)publish multiple locales

* fix(core): cleanup locale param types

* feat(content-manager): wip - multiple locale support

* Count draft relations across locales (#20116)

* feat(content-manager): publish multiple locales from CM route

* feat(core): multiple locales in document service discard draft

* feat(content-manager): use bulkpublish for locale publish

* feat(content-manager): use query from bulkpublish locales

* feat(content-manager): publishMany

* feat(content-manager): api test for bulk locale + document publish

* chore(content-manager): fix for build

* fix(core): avoid * in locale data

* chore(content-manager): pr amends and validation improvement

* feat(content-manager): use transaction in document manager publish many

* feat(core): throw when a non string locale is provided and not supported

* fix(core): doc service find many

* chore: clean up

* feat(content-manager): Select fields that require validation in availablelocales (#20156)

* feat(content-manager): wip - select more fields in availablelocales

* feat(content-manager): wip available locales using entity traversal

* feat(content-manager): wip test available locales returns fields with validation

* feat(content-manager): test available locales returns fields with validation

* fix(content-manager): ensure sensitive info not exposed in available statuses

* fix(content-manager): sanitize document metadata available status

* fix(content-manager): sanitize document metadata available status at controller level

* fix(content-manager): populate only for validatable fields

* chore: clean up

* chore: clean up

* chore: clean up

* fix(content-manager): build issues

* fix(content-manager): allow null locales

* fix(content-manager): history service (#20185)

* fix(content-manager): history service

* chore(content-manager): clean up

* fix: pr feedback

* chore: update actions deps

* chore: update utility deps

* chore: upgrade sentry

* chore: upgrade graphql-tools

* fix: http-errors ugprade

* chore: add fs-extra types where needed

* docs(typescript): type system cheat sheet

* chore!: remove deprecated verbose option from ts:generate-types

* chore: clean up

fix(content-manager) correctly count bulk publish results

* fix(content-manager): pr feedback and test improvements

* feat(i18n): bulk locale publish modal in CM edit view (#20069)

* feat(i18n): wip bulk locale publish modal

* fix(i18n): wip - fe bulk locale publish

* feat(content-manager): multi locale publish, integrate with backend and add basic e2e test

* feat(i18n): wip - display validation errors in bulk locale modal

* chore: clean up

* chore(i18n): design system changes

* feat(i18n): display correct status after publish and clean up error messaging

* feat(i18n): access onclose from modal body

* fix(i18n): selected locale change

* fix(i18n): locale table state

* fix(i18n): edit view e2e test

* chore(content-manager): validation tweak wip

* feat(i18n): cover validation cases in i18n e2e tests

* chore: clean up

* fix(i18n): edit view more document actions disabled state

* chore: feedback

* fix(i18n): send all params to publish many

* fix(i18n): place bulk locale publish 3rd in array

* fix(content-manager): validation error extraction

* fix(content-manager): pr feedback

* fix: build

* chore(content-manager): simplify exports

* chore(i18n): revert package

---------

Co-authored-by: Alexandre Bodin <bodin.alex@gmail.com>
Co-authored-by: Jean-Sébastien Herbaux <jean-sebastien.herbaux@epitech.eu>
Co-authored-by: Ben Irvin <ben.irvin@strapi.io>
jhoward1994 added a commit that referenced this pull request May 10, 2024
* fix: date comparison

* feat(core): document service publish multiple locales (#20046)

* feat(core): document service (un)publish multiple locales

* fix(core): cleanup locale param types

* feat(content-manager): wip - multiple locale support

* Count draft relations across locales (#20116)

* feat(content-manager): publish multiple locales from CM route

* feat(core): multiple locales in document service discard draft

* feat(content-manager): use bulkpublish for locale publish

* feat(content-manager): use query from bulkpublish locales

* feat(content-manager): publishMany

* feat(content-manager): api test for bulk locale + document publish

* chore(content-manager): fix for build

* fix(core): avoid * in locale data

* chore(content-manager): pr amends and validation improvement

* feat(content-manager): use transaction in document manager publish many

* feat(core): throw when a non string locale is provided and not supported

* fix(core): doc service find many

* chore: clean up

* feat(content-manager): Select fields that require validation in availablelocales (#20156)

* feat(content-manager): wip - select more fields in availablelocales

* feat(content-manager): wip available locales using entity traversal

* feat(content-manager): wip test available locales returns fields with validation

* feat(content-manager): test available locales returns fields with validation

* fix(content-manager): ensure sensitive info not exposed in available statuses

* fix(content-manager): sanitize document metadata available status

* fix(content-manager): sanitize document metadata available status at controller level

* fix(content-manager): populate only for validatable fields

* chore: clean up

* chore: clean up

* chore: clean up

* fix(content-manager): build issues

* fix(content-manager): allow null locales

* fix(content-manager): history service (#20185)

* fix(content-manager): history service

* chore(content-manager): clean up

* fix: pr feedback

* chore: update actions deps

* chore: update utility deps

* chore: upgrade sentry

* chore: upgrade graphql-tools

* fix: http-errors ugprade

* chore: add fs-extra types where needed

* docs(typescript): type system cheat sheet

* chore!: remove deprecated verbose option from ts:generate-types

* chore: clean up

fix(content-manager) correctly count bulk publish results

* fix(content-manager): pr feedback and test improvements

* feat(i18n): bulk locale publish modal in CM edit view (#20069)

* feat(i18n): wip bulk locale publish modal

* fix(i18n): wip - fe bulk locale publish

* feat(content-manager): multi locale publish, integrate with backend and add basic e2e test

* feat(i18n): wip - display validation errors in bulk locale modal

* chore: clean up

* chore(i18n): design system changes

* feat(i18n): display correct status after publish and clean up error messaging

* feat(i18n): access onclose from modal body

* fix(i18n): selected locale change

* fix(i18n): locale table state

* fix(i18n): edit view e2e test

* chore(content-manager): validation tweak wip

* feat(i18n): cover validation cases in i18n e2e tests

* chore: clean up

* fix(i18n): edit view more document actions disabled state

* chore: feedback

* fix(i18n): send all params to publish many

* fix(i18n): place bulk locale publish 3rd in array

* fix(content-manager): validation error extraction

* fix(content-manager): pr feedback

* fix: build

* chore(content-manager): simplify exports

* chore(i18n): revert package

---------

Co-authored-by: Alexandre Bodin <bodin.alex@gmail.com>
Co-authored-by: Jean-Sébastien Herbaux <jean-sebastien.herbaux@epitech.eu>
Co-authored-by: Ben Irvin <ben.irvin@strapi.io>

* fix(i18n): disable publish button after bulk locale publish

* fix(content-manager): ce e2e fix

* chore: typography

* fix(i18n): e2e edit view test

* fix: wip date comparison

* fix: wip date comparison

* fix(content-manager): simplify date comparison

* fix: clean up metadata api test

* chore: update api tests

* fix: draft versions must be ahead of published to be considered modified

* fix: published modified calculation

* fix: clean up

* fix: simplify draft and publish comparison

* chore: clean up

* fix: flaky fe tests

* fix: pr feedback

* fix(i18n): error message extraction in bulk locale modal

* chore: remove old publish action

* chore: pr feedback

* chore: refactor error types & validation messages

* chore: use anchor link for locale changes

* chore: fix releases

* fix: clean up

* chore: snapshot

---------

Co-authored-by: Marc-Roig <marc12info@gmail.com>
Co-authored-by: Alexandre Bodin <bodin.alex@gmail.com>
Co-authored-by: Jean-Sébastien Herbaux <jean-sebastien.herbaux@epitech.eu>
Co-authored-by: Ben Irvin <ben.irvin@strapi.io>
Co-authored-by: Josh <37798644+joshuaellis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants