Skip to content

LG-9449: Implement and configure ArcGIS API token refresh job#8692

Merged
NavaTim merged 2 commits intomainfrom
tbradley/LG-9449-arcgis-token-refresh-redux
Jun 29, 2023
Merged

LG-9449: Implement and configure ArcGIS API token refresh job#8692
NavaTim merged 2 commits intomainfrom
tbradley/LG-9449-arcgis-token-refresh-redux

Conversation

@NavaTim
Copy link
Contributor

@NavaTim NavaTim commented Jun 28, 2023

🎫 Ticket

🛠 Summary of changes

  • Create scheduled job to refresh ArcGIS token regularly before it expires
    • Cron is configurable because while the default expiration is 1 hour, the server can use a different value.

This intentionally excludes the retry and sliding window logic that was attempted in #8662 and #8579. These features can help with availability, but they contribute enough complexity that it doesn't make sense to shoehorn them into the same story.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Deploy with job enabled
  • Wait for job window to pass
  • Check that analytics shows job logs
  • Check that analytics shows API call from job
  • Check that the PO search does not attempt to fetch a different token
  • Disable job
  • Wait for job window to pass
  • Check that analytics does not show job logs
  • Check that analytics does not show API call from job
  • Re-enable job

changelog: Internal, In-person proofing, Refresh ArcGIS token in job
@NavaTim NavaTim requested review from a team, JackRyan1989, eileen-nava, racingspider and tomas-nava and removed request for a team, JackRyan1989, eileen-nava and racingspider June 28, 2023 23:37
@NavaTim NavaTim force-pushed the tbradley/LG-9449-arcgis-token-refresh-redux branch from 6c24270 to 7e41b24 Compare June 28, 2023 23:50
@gina-yamada
Copy link
Contributor

LGTM - Code is well tested, code comments are helpful, refresh enabled defaults to false except for in production

@NavaTim NavaTim merged commit 7a9da9c into main Jun 29, 2023
@NavaTim NavaTim deleted the tbradley/LG-9449-arcgis-token-refresh-redux branch June 29, 2023 17:26
Comment on lines +172 to +178
arcgis_token: (if IdentityConfig.store.arcgis_api_refresh_token_job_enabled
{
class: 'ArcgisTokenJob',
cron: IdentityConfig.store.arcgis_api_refresh_token_job_cron,
}
end),
}.compact
Copy link
Contributor

Choose a reason for hiding this comment

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

two ideas:

  1. what if we broke this into a second statement to avoid some goofy indentation here?
  2. what if we used .select(&:present?)
Suggested change
arcgis_token: (if IdentityConfig.store.arcgis_api_refresh_token_job_enabled
{
class: 'ArcgisTokenJob',
cron: IdentityConfig.store.arcgis_api_refresh_token_job_cron,
}
end),
}.compact
}
if IdentityConfig.store.arcgis_api_refresh_token_job_enabled
config.good_job.cron[:arcgis_token] = {
class: 'ArcgisTokenJob',
cron: IdentityConfig.store.arcgis_api_refresh_token_job_cron,
}
end
Suggested change
arcgis_token: (if IdentityConfig.store.arcgis_api_refresh_token_job_enabled
{
class: 'ArcgisTokenJob',
cron: IdentityConfig.store.arcgis_api_refresh_token_job_cron,
}
end),
}.compact
arcgis_token: {
class: 'ArcgisTokenJob',
cron: IdentityConfig.store.arcgis_api_refresh_token_job_cron,
} && IdentityConfig.store.arcgis_api_refresh_token_job_enabled,
}.select(&:present?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was deliberately trying to avoid the second statement you mentioned, though I agree the indentation is goofy.

The .select(&:present?) looks easier to understand, but it doesn't appear to work as shown based on my testing in the Rails console.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah whoops yes it's this:

> { a: false, b: 1 }.select { |_k, v| v.present? }
=> {:b=>1}

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