-
Notifications
You must be signed in to change notification settings - Fork 452
Shipping Features endpoint fix #5538
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this change! Reducing the API response to only the feature URL makes a lot of sense for improving performance and readability. The URL change also helps simplify the endpoint.
The code looks good to me. I've verified that the logic changes in shipping_features_api.py
are correctly reflected in the updated tests in shipping_features_api_test.py
.
This PR was reviewed by Gemini.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after one change.
api/shipping_features_api.py
Outdated
|
||
if feature.feature_type == FEATURE_TYPE_CODE_CHANGE_ID: | ||
# PSA features do not require intents or approvals. | ||
feature_dict = converters.feature_entry_to_json_verbose(feature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still need feature_dict? Avoiding this call would avoid more NDB operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch - I did not remove this line
* less feature information in shipping features API * fix tests * revert temporary test change * remove unused line
Part of #5498
This change adds two changes to the shipping features endpoint: