Skip to content

LG-10062 Allow updater to update a single service provider #9032

Merged
Sgtpluck merged 6 commits intomainfrom
dmm/LG-10062/sync-specific-service-providers
Aug 21, 2023
Merged

LG-10062 Allow updater to update a single service provider #9032
Sgtpluck merged 6 commits intomainfrom
dmm/LG-10062/sync-specific-service-providers

Conversation

@Sgtpluck
Copy link
Contributor

@Sgtpluck Sgtpluck commented Aug 17, 2023

🎫 Ticket

https://cm-jira.usa.gov/browse/LG-10062

🛠 Summary of changes

When a partner on the dashboard side updates their sandbox configurations, those are synced with the int environment. The updater historically has pinged the dashboard to get a full list of service provider and then synced through the entire list. This no longer scales for the number of sandbox configurations we have and has led to many timeouts.

This change updates the updater to accept a service_provider param, and uses that param to update a single service_provider. It does not remove/alter the existing code, which may be in use in other parts of the application.

It has this associated PR on the Dashboard application.

Open questions:

  • Do we still want/need the conditionals around the native attribute? It appears that that is a deprecated attribute, but I didn't want to remove it without more context.
    • answer: the dashboard does not send over the deprecated native attribute. will follow up in a separate PR to remove it
  • Do we want to remove the original code, which updates all the service providers? It has two references in the Dashboard application, one which has been updated in the associated PR, and the other is in the API and may not be in use.
    • answer: this code is still in use! there is a button on the service_providers all view. we should move this code out of the API section as part of our tech debt work as the update requires a session.
  • I broke the existing pattern of having the IDP ping the dashboard for the attributes because it seemed unnecessarily convoluted, but I can't find the reason that that pattern was created in the first place. If there is a known reason that having the dashboard submit a POST to the IDP that then does a GET request to the dashboard, I can update the code to make that work.

Comment on lines 31 to 33
Copy link
Contributor

Choose a reason for hiding this comment

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

are we doing a form post?

if we switched to JSON encoding of the body when we post, we'd likely preserve boolean true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah thanks, fixed in 8ac09c9 (#9032)

Copy link
Contributor

Choose a reason for hiding this comment

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

Rails will automatically parse JSON when entire POST bodies are JSON (if the Content-Type header is set correctly) so I don't think we need to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm ok i hadn't updated the headers of the original request, i'll do that and see if it works!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed here 2be634b (#9032) and on the dashboard here: 18F/identity-dashboard@c4aa305 (#663)

@Sgtpluck Sgtpluck force-pushed the dmm/LG-10062/sync-specific-service-providers branch 2 times, most recently from 9960f3f to 3143ce1 Compare August 18, 2023 18:10
@Sgtpluck Sgtpluck force-pushed the dmm/LG-10062/sync-specific-service-providers branch from 3143ce1 to 2cad32d Compare August 18, 2023 19:09
@Sgtpluck Sgtpluck merged commit a40818f into main Aug 21, 2023
@Sgtpluck Sgtpluck deleted the dmm/LG-10062/sync-specific-service-providers branch August 21, 2023 14:57
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.

4 participants