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

feat(history): capture content types history #19343

Merged
merged 10 commits into from Jan 29, 2024
Merged

Conversation

remidej
Copy link
Contributor

@remidej remidej commented Jan 26, 2024

What does it do?

  • creates a history service
  • creates a document middleware that automatically saves history when entries are created or updated

Why is it needed?

To make history

How to test it?

  • Use a database explorer and look up the strapi_history_versions table
  • Update an entry in the admin, you should see a new row in the table with all the columns filled
  • Same thing for entry creation
  • No other actions should create new rows

@remidej remidej added source: core:content-manager Source is core/content-manager package pr: feature This PR adds a new feature labels Jan 26, 2024
@remidej remidej self-assigned this Jan 26, 2024
Copy link
Contributor

@Marc-Roig Marc-Roig left a comment

Choose a reason for hiding this comment

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

🚀

Comment on lines +21 to +29
// Check if store already exists
const oldStore = storage.getStore();

return storage.run<ReturnType<TCallback>, void[]>(
{ trx: store, commitCallbacks: [], rollbackCallbacks: [] },
{
trx: store,
commitCallbacks: oldStore?.commitCallbacks || [],
rollbackCallbacks: oldStore?.rollbackCallbacks || [],
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix was found by @Marc-Roig, it's because nested transactions weren't inheriting the parent transaction context. Marc will ship another dedicated PR with tests but bundling the change in this PR too allows us to move on with this PR

Copy link
Contributor

@markkaylor markkaylor left a comment

Choose a reason for hiding this comment

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

Tested locally and it's working.

Comment on lines +79 to +80
createdAt: new Date(),
createdBy: strapi.requestContext.get()?.state?.user.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Already discussed on slack but put this here anyway. I don't know how it works now but in v4 these were already set when using content-types, well you get them using an util:

const releaseWithCreatorFields = await setCreatorFields({ user })(releaseData);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I'll look more at what this does when I implement the model API

@remidej remidej merged commit 4e73788 into v5/history Jan 29, 2024
7 checks passed
@remidej remidej deleted the history/service branch January 29, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: feature This PR adds a new feature source: core:content-manager Source is core/content-manager package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants