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
feat(history): restore version #20102
Conversation
packages/core/content-manager/admin/src/history/components/VersionHeader.tsx
Show resolved
Hide resolved
packages/core/content-manager/server/src/history/controllers/history-version.ts
Outdated
Show resolved
Hide resolved
packages/core/content-manager/server/src/history/controllers/history-version.ts
Show resolved
Hide resolved
packages/core/content-manager/admin/src/history/components/VersionHeader.tsx
Outdated
Show resolved
Hide resolved
packages/core/content-manager/admin/src/history/services/historyVersion.ts
Outdated
Show resolved
Hide resolved
packages/core/content-manager/server/src/history/services/history.ts
Outdated
Show resolved
Hide resolved
packages/core/content-manager/server/src/history/services/history.ts
Outdated
Show resolved
Hide resolved
History has only a single service, which is now in charge of :
Would it make sense to split the history service into two?
It could simplify method naming and split the services concerns |
packages/core/content-manager/server/src/history/controllers/history-version.ts
Outdated
Show resolved
Hide resolved
packages/core/content-manager/server/src/history/controllers/history-version.ts
Outdated
Show resolved
Hide resolved
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/core/content-manager/server/src/history/controllers/history-version.ts
Outdated
Show resolved
Hide resolved
unrelated to this PR, but you might want to change the following line in the history service: // before
const localesService = strapi.plugin('i18n').service('locales');
// after
const localesService = strapi.plugin('i18n')?.service('locales');
// Return or handle it differently
if (!localesService) return if i18n is not enabled that might break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're ready to merge and treat potential issues as bug fixes PRs
@remidej I'll push to address Marc's feedback for:
Then good to merge with follow up PRs for bug fixes + tests |
@remidej and/or @Marc-Roig could you please test the validation of the request body and the validation of update permissions. For the update permissions here I am getting 403 when I don't the permissions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again ✅
The failing FE test is unrelated. Merging. |
What does it do?
Why is it needed?
How to test it?
Test relations and media
Unknown fields:
Not supported:
NOTE
To ensure this work makes it into the beta blitz on monday tests will be handled in another PR CS-752