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

Cs/sc 3360 support road network algorithm #34024

Closed
wants to merge 16 commits into from

Conversation

Charl1996
Copy link
Contributor

Product Description

Demo video

The disbursement algorithm, "Road Network Algorithm", makes use of the Mapbox Matrix API. This PR adds the ability for a project to add their own mapbox token on the Geospatial Settings page.

image

Once the FF is enabled for a project, the new disbursement algorithm will appear on the Geospatial Settings page. Once the algorithm is selected, two new HTML elements will appear on the screen:

  1. Text (password) input prompting the user for their mapbox token.
  2. A button ("Test API Key") which the user can click to make a sample request to Mapbox in order to verify that the token works.

Note that the user don't have to click the "Test API Key" button in order to save the token; the button exists purely to test the token.

Technical Summary

Ticket

Noteworthy items:

  1. The api token is stored in postgres as an encrypted string, but returned to the front end in plaintext (although rendered as a password) when loading the Settings page.
  2. The "Test API Key" button sample API call happens purely in the browser; I decided we don't have to have a dedicated endpoint just for the server to make an API call to Mapbox if we could simply use ajax.

Feature Flag

New FF: SUPPORT_ROAD_NETWORK_DISBURSEMENT_ALGORITHM

Safety Assurance

Safety story

Tested this feature on my local machine.

Automated test coverage

  • Tests for view
  • Tests for model

QA Plan

QA planned

Migrations

  • The migrations in this code can be safely applied first independently of the code

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@Charl1996 Charl1996 added awaiting QA QA in progress. Do not merge product/feature-flag Change will only affect users who have a specific feature flag enabled labels Jan 24, 2024
@dimagimon dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Jan 24, 2024
Copy link
Contributor

@zandre-eng zandre-eng 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, just a few minor comments.

corehq/apps/geospatial/forms.py Outdated Show resolved Hide resolved
corehq/apps/geospatial/static/geospatial/js/geo_config.js Outdated Show resolved Hide resolved
corehq/apps/geospatial/models.py Outdated Show resolved Hide resolved
corehq/apps/geospatial/static/geospatial/js/geo_config.js Outdated Show resolved Hide resolved
corehq/apps/geospatial/static/geospatial/js/geo_config.js Outdated Show resolved Hide resolved
corehq/apps/geospatial/routing_solvers/pulp.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mkangia mkangia left a comment

Choose a reason for hiding this comment

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

Quick question @Charl1996,
why are we putting this behind a new feature flag? Can't we simply include it in the feature itself.
If its new, can we simply add "(unstable)" or something similar in the name in the dropdown when user is selecting it to let them know it is still in development.
Since we already have a feature flag for GEOSPATIAL, this is anyway being used by known projects.

corehq/apps/geospatial/models.py Outdated Show resolved Hide resolved
corehq/apps/geospatial/models.py Outdated Show resolved Hide resolved
corehq/apps/geospatial/models.py Outdated Show resolved Hide resolved
corehq/apps/geospatial/static/geospatial/js/geo_config.js Outdated Show resolved Hide resolved
corehq/apps/geospatial/tests/test_views.py Show resolved Hide resolved
@Charl1996
Copy link
Contributor Author

why are we putting this behind a new feature flag?

The reason is simply because there's additional costs and considerations involved when using this algorithm. I suppose we don't have to. Let met check with Mary.

Copy link
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

Thanks for all the effort on this @Charl1996, looks good!

Copy link
Contributor

@mkangia mkangia left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up on using the plaintext property @Charl1996
Appreciate the flexibility.

I assume this has not been deployed on any environment so can we simply club the migrations to keep only one final migration.

else:
self._api_token = None
self.api_token = None
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I am thinking that this else block is implicitly resetting the api_token.

I would think that this function would encrypt and store the value and if it is not able to should it

  1. do nothing
  2. raise an error

I would prefer 2. More specifically:

if value is None:
    self.api_token = None
elif value and not value.startswith(f'${ALGO_AES}$'):
    # encrypt and store
else:
    raise Exception("Unexpected value received for plaintext api token")

Let me know what you think.

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 see what you're meaning and I concur. I'll update!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

335f4c8

The setter now also handles the case where you set a non-string value, say, 1234. You also can't set an empty api_token. The token can only ever be either

  1. A string value (not empty)
  2. null

choices = self.fields['selected_disbursement_algorithm'].choices
choices.append(self.ROAD_NETWORK_ALGORITHM_OPTION)
self.fields['selected_disbursement_algorithm'].choices = choices

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

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 the PR description is still referring to this and needs to be updated.

self.api_token = f'${ALGO_AES}${ciphertext}'
else:
raise Exception("Unexpected value set for plaintext api token")
except AttributeError:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the improvement @Charl1996
Can you share what will raise the exception AttributeError?

Additionally, possible to keep the two messages different? If one gets an error, it wont be clear where it came from.

Copy link
Contributor

Choose a reason for hiding this comment

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

When raising exceptions its nice to be explicit what code would raise the exception. It's not clear to me here what will raise the exception AttributeError, though if only the necessary code is kept in the try block, its easier to locate.

@Charl1996
Copy link
Contributor Author

Closing in favour of #34081 (cleaned up the commits a bit).

@Charl1996 Charl1996 closed this Feb 2, 2024
@mkangia mkangia deleted the cs/SC-3360-support-road-network-algorithm branch February 2, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting QA QA in progress. Do not merge product/feature-flag Change will only affect users who have a specific feature flag enabled reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants