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

[DPC-4442] Remove Endpoint #2396

Merged
merged 33 commits into from
Feb 3, 2025
Merged

[DPC-4442] Remove Endpoint #2396

merged 33 commits into from
Feb 3, 2025

Conversation

ashley-weaver
Copy link
Contributor

🎫 Ticket

https://jira.cms.gov/browse/DPC-4442

🛠 Changes

EndpointResource and all associated code removed.

ℹ️ Context

Organization endpoints are not being used, and can be removed.

🧪 Validation

Tests updated.

@@ -170,9 +163,9 @@ private Map<String, Reference> seedPatientBundle(FHIREntityConverter converter,
})
.map(entity -> patientEntityToRecord(context, entity))
.peek(context::executeInsert)
.forEach(record -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"record" is a protected term

@ashley-weaver ashley-weaver requested a review from a team January 27, 2025 21:09
@ashley-weaver ashley-weaver marked this pull request as ready for review January 27, 2025 21:14
Copy link
Contributor

@MEspositoE14s MEspositoE14s left a comment

Choose a reason for hiding this comment

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

Happy to see it go!

@jdettmannnava
Copy link
Contributor

Have we made sure the web and admin still work?

</changeSet>

<changeSet id="drop-organization-endpoints-table" author="ashley-weaver">
<dropTable tableName="ORGANIZATION_ENDPOINTS"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to test this migration in a deployed environment before merging or do you have a high level of confidence making these changes? @ashley-weaver

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that cleaning up the databases as a separate ticket is probably safer.

@ashley-weaver ashley-weaver marked this pull request as draft January 28, 2025 14:20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we even use this file anywhere? can it be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like junk to me. It was added as part of a token validation, but doesn't seem to be used anywhere. grep didn't find it.

@ashley-weaver ashley-weaver marked this pull request as ready for review January 29, 2025 16:24
Copy link
Contributor

@jdettmannnava jdettmannnava left a comment

Choose a reason for hiding this comment

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

Looks good!
I don't think the edit-registered-organization workflow does anything anymore, so it can be removed from dpc-admin.
Also, we might want to leave the table in in case we need to roll back.

<div class="ds-u-margin-bottom--1">
Status: <%= @organization.registered_organization.fhir_endpoint.status.titleize %>
</div>

<%= link_to "Edit", edit_organization_registered_organization_path(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably go, as the only editing is on the endpoint.

@@ -0,0 +1,12 @@
class DropFhirEndpointsTable < ActiveRecord::Migration[7.2]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am kind of thinking we should leave this in case we need to roll back.

Copy link
Contributor

@jdettmannnava jdettmannnava left a comment

Choose a reason for hiding this comment

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

LGTM. Some minor tweaks if you feel like it!

@ashley-weaver ashley-weaver merged commit 60b6a60 into main Feb 3, 2025
8 checks passed
@ashley-weaver ashley-weaver deleted the aweaver/remove-endpoint branch February 3, 2025 13:08
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.

None yet

4 participants