Skip to content

feat: Support change logs for bulk insert #166

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

Merged
merged 5 commits into from
Apr 24, 2025
Merged

Conversation

akshay11298
Copy link
Contributor

@akshay11298 akshay11298 commented Mar 29, 2025

Addresses- #145

  • Capture change log for each row
  • Add test

Currently on bulk insert diff comes as an array instead of object, so extracting properties like ID (for entity key), or old_value (for change detection) returns undefined, since they need to be extracted from element of array. Updated the logic to iterate over diff if array, to treat each diff (row) individually, accumulating the changes and doing a bulk insert in ChangeLog entity.

@akshay11298
Copy link
Contributor Author

Hi @nkaputnik @Sv7enNowitzki , I have created this PR to resolve the issue mentioned. This is my first time contributing to the repo. If I have missed anything, please let me know.

Thanks
Akshay

@Sv7enNowitzki Sv7enNowitzki self-requested a review March 30, 2025 14:35
Copy link
Collaborator

@Sv7enNowitzki Sv7enNowitzki left a comment

Choose a reason for hiding this comment

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

Hello @akshay11298 ,

As I understand it, this plugin is designed following the standard behavior of Fiori elements. What I mean is that in standard Fiori element pages, processing is generally done from the perspective of updating a single root entity, which means that the situation you mentioned wasn't previously considered. This is indeed a shortcoming, so I made some changes based on your commit and expanded the unit tests to increase coverage. Thank you for your contribution.😄

Best Regards,
Wenjun

@Sv7enNowitzki Sv7enNowitzki requested a review from nkaputnik March 30, 2025 14:47
@nkaputnik
Copy link
Contributor

Hello @Sv7enNowitzki
I checked the stuff and don't see any road blocks. Should I merge this to main?

@Sv7enNowitzki
Copy link
Collaborator

Hello @Sv7enNowitzki I checked the stuff and don't see any road blocks. Should I merge this to main?

Hi @nkaputnik ,
Yes, I have already modified and approved this PR earlier. If there are no issues on your end, we can proceed with the merge.

@nkaputnik nkaputnik merged commit ef1d62e into cap-js:main Apr 24, 2025
3 checks passed
@akshay11298
Copy link
Contributor Author

Thanks @nkaputnik @Sv7enNowitzki for reviewing the PR and adding this feature 😄

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.

3 participants