-
Notifications
You must be signed in to change notification settings - Fork 36
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
Mmt-3116: Update MMT to use the new "Order Option" API #955
Conversation
@@ -54,30 +131,95 @@ def create | |||
end | |||
end | |||
|
|||
def show | |||
def deprecate_order_option |
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.
I think a comment may be needed something along the lines of "deprecate AKA remove order option" or something to that affect. To the uninitiated it looks like you renamed a function you want to delete latter. Everyone involved probably understands you, but I'm sure someone in the future will be confused.
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.
No, this function is to do deprecating an order option (not it self is deprecated)
end | ||
|
||
def delete_order_option | ||
response = cmr_client.remove_order_option(native_id: params[:id], provider_id: current_user.provider_id, token: token) |
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.
I am slightly concerned about us deleting the order-options
. When we migrated the order-options from legacy services we associated them to collections and services by adding them as part of the data payload, if we delete the order-option from CMR it will still remain on the payload associated between the service and collection. Querying for an order-option with the concept-id in that payload will of course return no results.
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.
I don't think that is necessarily part of this ticket's scope though
No description provided.