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

2937 rfc mobile endpoint naming convention #41036

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

aherzberg
Copy link
Contributor

@aherzberg aherzberg commented May 6, 2022

RFC for versioning endpoint of naming consistency

get '/military-service-history', to: 'military_information#get_service_history'
get '/payment-history', to: 'payment_history#index'
get '/payment-information/benefits', to: 'payment_information#index'
put '/payment-information/benefits', to: 'payment_information#update'
Copy link
Contributor Author

@aherzberg aherzberg May 6, 2022

Choose a reason for hiding this comment

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

Maybe make a payment parent product?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or put the payment information endpoints under benefits

Copy link
Contributor

Choose a reason for hiding this comment

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

Payment information isn't a descriptive name to begin with 😄 as it's easily confused with payment history (which I just did before editing this comment 😀). But yes, I think it should go under benefits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is payment history exclusively used for benefits as well?

get '/letters/beneficiary', to: 'letters#beneficiary'
post '/letters/:type/download', to: 'letters#download'
get '/messaging/health/messages/signature', to: 'messages#signature'
get '/military-service-history', to: 'military_information#get_service_history'
Copy link
Contributor

Choose a reason for hiding this comment

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

/user/military-service-history ?

get '/disability-rating', to: 'disability_rating#index'
get '/health/immunizations', to: 'immunizations#index'
get '/health/locations/:id', to: 'locations#show'
get '/letters', to: 'letters#index'
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure letters are only about benefits.

get '/push/:endpoint_sid/prefs', to: 'push_notifications#get_prefs'
put '/push/:endpoint_sid/prefs', to: 'push_notifications#set_pref'

scope :messaging do
Copy link
Contributor

Choose a reason for hiding this comment

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

Messaging is health; a user has to be registered as a patient at a VA health facility to use it. So I think we can flip the scopes so the path ends up being health/messaging.

get '/appointments/va/eligibility', to: 'veterans_affairs_eligibility#show'
get '/appointments/facility/eligibility', to: 'facility_eligibility#index'
post '/appointment', to: 'appointments#create'
get '/community-care-providers', to: 'community_care_providers#index'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe /appointments/community-care-providers ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Andrew and I discussed that when I created the endpoint. Our rationale for not doing it is that there could be some theoretical reason that we might need to use providers for a reason not directly linked to appointments (like showing a map of locations or something). I think that's very unlikely and I'm happy with it either way. It sounds like we're trying to do a better job of organizing our module, which is a good thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I liked the idea at the time but now for the sake of simplicity it does seem cleaner put appointments in the url. Maybe we just keep it as a separate controller in case of the unlikely event that we need to implement more actions for cc providers? probably should do the same with the facility info endpoint as well then.

get '/appointments/community-care/:service_type/eligibility', to: 'community_care_eligibility#show'
get '/appointments/va/eligibility', to: 'veterans_affairs_eligibility#show'
get '/appointments/facility/eligibility', to: 'facility_eligibility#index'
post '/appointment', to: 'appointments#create'
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be plural? Even though it's creating a single appointment, the REST convention is that a post to a plural endpoint means one item should be added to the collection.

get '/appointments', to: 'appointments#index'
put '/appointments/:id/cancel', to: 'appointments#cancel'
get '/appointments/community-care/:service_type/eligibility', to: 'community_care_eligibility#show'
get '/appointments/va/eligibility', to: 'veterans_affairs_eligibility#show'
Copy link
Contributor

Choose a reason for hiding this comment

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

/appointments/eligibility/va, /appointments/eligibility/facility ?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's community care too 😁 and although community-care could be a parent I think it has to do more with 'can' rather than 'where' it could also fall under eligibility /appointments/eligibility/community-care ?

post '/claim/:id/request-decision', to: 'claims_and_appeals#request_decision'
end

get '/maintenance-windows', to: 'maintenance_windows#index'
Copy link
Contributor

Choose a reason for hiding this comment

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

/maintenance-windows applies to all services, could it go to the root?

Copy link
Contributor Author

@aherzberg aherzberg left a comment

Choose a reason for hiding this comment

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

Did another pass of the endpoints. @kreek addresses all of your feedback. made some additional changes:

  • put disability ratings within benefits
  • made claim and appeal endpoints plural

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