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.

Sign up for GitHub

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

Jump to bottom

Bulk delete enhancements #3505

Merged
merged 42 commits into from
Jan 4, 2024
Merged

Bulk delete enhancements #3505

merged 42 commits into from
Jan 4, 2024

Conversation

LTA-Thinking
Copy link
Collaborator

Description

Adds audit logging to delete operations
Improves bulk delete performance

Related issues

Addresses User Story 104736: [Needs to be done prior to GA] Audit event logs need to be populated with delete information

Testing

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch

@LTA-Thinking LTA-Thinking added this to the S123 milestone Sep 1, 2023
@LTA-Thinking LTA-Thinking added Enhancement Enhancement on existing functionality. Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs labels Sep 1, 2023
@LTA-Thinking LTA-Thinking modified the milestones: S123, S125 Oct 9, 2023
@LTA-Thinking
Copy link
Collaborator Author

/azp run

@LTA-Thinking LTA-Thinking marked this pull request as ready for review December 5, 2023 16:22
@LTA-Thinking LTA-Thinking requested a review from a team as a code owner December 5, 2023 16:22
mikaelweave
mikaelweave requested changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@mikaelweave mikaelweave left a comment

Choose a reason for hiding this comment

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

A couple changes around rest file / operation definitions. Others are responses to reading the code and questions I have which may need changes or just responses.

Great quality PR here - especially the reuse of the soft delete work I added. 🦾

src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/BulkDeleteProcessingJob.cs Outdated Show resolved Hide resolved
src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/BulkDeleteOrchestratorJob.cs Outdated Show resolved Hide resolved
src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/Handlers/GetBulkDeleteHandler.cs Show resolved Hide resolved
src/Microsoft.Health.Fhir.Api/Features/Routing/KnownRoutes.cs Show resolved Hide resolved
src/Microsoft.Health.Fhir.Api/Features/Routing/KnownRoutes.cs Show resolved Hide resolved
src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/BulkDeleteDefinition.cs Outdated Show resolved Hide resolved
@mikaelweave
Copy link
Contributor

Another question about bulk-delete in general from local testing - would an Integer64 be better suited than a FhirDecimal for the value in the parameters in the response?

I really don't think it matters at all, but once this is GA we can't change it. Integer64 is a bit clearer since you cannot delete part of a resource.

mikaelweave
mikaelweave previously approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@mikaelweave mikaelweave left a comment

Choose a reason for hiding this comment

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

Awesome job on this PR! 🦾 Approved pending build/tests.

mikaelweave
mikaelweave previously approved these changes Dec 8, 2023
View reviewed changes
mikaelweave
mikaelweave approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@mikaelweave mikaelweave left a comment

Choose a reason for hiding this comment

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

🦾🚀

@LTA-Thinking LTA-Thinking merged commit f0776e2 into main Jan 4, 2024
6 checks passed
@LTA-Thinking LTA-Thinking deleted the personal/rojo/bulk-delete-logging branch January 4, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikaelweave mikaelweave mikaelweave approved these changes

Assignees
No one assigned
Labels
Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement Enhancement on existing functionality.
Projects
None yet
Milestone
S125
Development

Successfully merging this pull request may close these issues.
2 participants
@LTA-Thinking @mikaelweave