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

Adds $purge-history operation #2022

Merged
merged 5 commits into from
Jun 18, 2021
Merged

Adds $purge-history operation #2022

merged 5 commits into from
Jun 18, 2021

Conversation

brendankowitz
Copy link
Member

@brendankowitz brendankowitz commented Jun 14, 2021 •
edited
Loading

Description

Adds support for $purge-history which can remove previous versions of a resource. This operation has been implemented as a variation of hard-delete since it is permanently removing data.

Related issues

Addresses [issue #].

Testing

Adds E2E tests

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 50 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, Dependencies, Enhancement, or New-Feature
  • Tag the PR with Azure API for FHIR if this will release to the managed service
  • Review squash-merge requirements

Semver Change (docs)

Patch

@brendankowitz brendankowitz requested a review from a team as a code owner June 14, 2021 17:07
LTA-Thinking
LTA-Thinking reviewed Jun 14, 2021
View reviewed changes
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/DeleteTests.cs Outdated
// Delete the resource.
await _client.DeleteAsync($"{nameof(Observation)}/{resourceId}/$purge");

// Subsequent read on the resource should return Gone.
Copy link
Collaborator

@LTA-Thinking LTA-Thinking Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This comment doesn't match the check that is happening after it. It looks like you are checking that the version id from a read matches the updated resource.

@LTA-Thinking
Copy link
Collaborator

There are tests for soft and hard delete in multiple test files. Do you think it is worthwhile to add purge to these test files as well?

Ivanidzo4ka
Ivanidzo4ka reviewed Jun 14, 2021
View reviewed changes
src/Microsoft.Health.Fhir.Api/Features/Routing/KnownRoutes.cs Outdated
@@ -58,6 +58,9 @@ internal class KnownRoutes
public const string Everything = "$everything";
public const string PatientEverythingById = "Patient/" + IdRouteSegment + "/" + Everything;

public const string Purge = "$purge";
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

$purge

PurgeHistory maybe?
I would expect recent version to be deleted from purge as well. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this suggestion :) PurgeHistory makes more sense to me too.

@brendankowitz brendankowitz force-pushed the personal/bkowitz/purge-operation branch from 9712203 to 16c409d Compare June 14, 2021 21:55
@brendankowitz brendankowitz changed the title Adds $purge operation Adds $purge-history operation Jun 15, 2021
@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Jun 15, 2021 •
edited
Loading 

(12, 'started')

13?

In reply to: 861824514

In reply to: 861824514

Refers to: src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Migrations/13.sql:9 in 88a06b4. [](commit_id = 88a06b4, deletion_comment = False)

@brendankowitz brendankowitz force-pushed the personal/bkowitz/purge-operation branch from b8c8367 to 8f0e3e1 Compare June 15, 2021 22:03
@brendankowitz brendankowitz force-pushed the personal/bkowitz/purge-operation branch from 8f0e3e1 to 9f971f8 Compare June 15, 2021 22:06
Ivanidzo4ka
Ivanidzo4ka reviewed Jun 16, 2021
View reviewed changes
src/Microsoft.Health.Fhir.Api/Features/Routing/KnownRoutes.cs
@@ -58,6 +58,9 @@ internal class KnownRoutes
public const string Everything = "$everything";
public const string PatientEverythingById = "Patient/" + IdRouteSegment + "/" + Everything;

public const string PurgeHistory = "$purge-history";
public const string PurgeHistoryResourceTypeById = ResourceTypeById + "/" + PurgeHistory;
Copy link
Contributor

Choose a reason for hiding this comment

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

PurgeHistoryResourceTypeById

To make it proper operations we need:
-Capability statement
-json file describing operation
-route which returns that json

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I mentioned that we should look at making operations a little simpler to add :)

Copy link

@GitForceJedi GitForceJedi Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Were they made simpler to add? What is the minimum required to add an operation, if all of the FHIR resources are available?

When asking the question I was pointed here. But this seems to have additional changes that may not be required for other operations.

Ivanidzo4ka
Ivanidzo4ka approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@brendankowitz brendankowitz merged commit 625ee7e into main Jun 18, 2021
@brendankowitz brendankowitz deleted the personal/bkowitz/purge-operation branch June 18, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GitForceJedi GitForceJedi GitForceJedi left review comments

@CaitlinV39 CaitlinV39 CaitlinV39 left review comments

@LTA-Thinking LTA-Thinking LTA-Thinking left review comments

@Ivanidzo4ka Ivanidzo4ka Ivanidzo4ka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brendankowitz @LTA-Thinking @Ivanidzo4ka @CaitlinV39 @GitForceJedi