Swagger documentation updates for services.bcpl.info

baltimorecounty / BCPL-assets

Client side assets for the Baltimore County Public Library website

4 stars 0 forks source link

Swagger documentation updates for services.bcpl.info #609

Closed danfox01 closed 5 years ago

danfox01 commented 6 years ago

Three things may need to be updated, I think. Let me know your thoughts, @martypowell .

I believe that this endpoint is used for the autocomplete. Can you confirm that, and if so -- does it make sense for us to identify that in the endpoint description?
The Swiftype endpoints (this and this) are no longer in use. How should we handle? Remove entirely, mark as depricated, etc?
The Get Carousel endpoint description is incomplete. I think it should say "Get a list of books or other materials from a Polaris record set."

martypowell commented 6 years ago

@danfox01

it is used for autocomplete, but the swagger docs, should not reflect how we are using the data we are consuming, just what the endpoint provides
I removed this code, and submitted for code review. I left the part of the code that specified how the search should work aka endpoints, so that we can reuse if needed
Description updated and submitted for code review

danfox01 commented 6 years ago

Great, thanks @martypowell. Keep me posted on next steps after review.

danfox01 commented 5 years ago

@martypowell and @tmccoy529, this is an old update to our Swagger documentation that was last updated in Nov pending code review. Does it make sense to try to pack these updates with any changes we're making to services for BCPL event sorting fix? (#615)

danfox01 commented 5 years ago

@danfox01 will verify docs in testservices

danfox01 commented 5 years ago

@martypowell @tmccoy529, looks like these did not make it to testservices. Let me know if you need me to summarize the changes that need to occur since it's been awhile since the issue was first logged.

tmccoy529 commented 5 years ago

@danfox01 I'll take a look and update when its complete.

tmccoy529 commented 5 years ago

@danfox01 I'm looking into this and all the changes to the original issue seem to have been resolved. Are there any other endpoints you feel don't have good enough descriptions? The alerts notification is listed as this "Get the highest ranked alert to display in banner from bcpl.info". Everything else to me looks like its resolved but wanted to check with you before I move this into the ready to release tab.

danfox01 commented 5 years ago

@tmccoy529 this looks good to me. One minor grammar change for /api/structured-content/blog should be "its" not "it's".

Otherwise, it's great. @martypowell can take a quick look too?

martypowell commented 5 years ago

looks good to me, there is a small error at the bottom of the page, I would look into maybe what that is displaying.

@danfox01 @tmccoy529

tmccoy529 commented 5 years ago

@danfox01 @martypowell I'll fix the grammar issue and look into that error.

danfox01 commented 5 years ago

@tmccoy529 will check with @martypowell to determine if this can be released despite the error

danfox01 commented 5 years ago

Per @martypowell: this is good for release. I'll add a separate issue to look into the error more, but let's not hold up the other stuff.

martypowell commented 5 years ago

@danfox01 i think this is in prod?

danfox01 commented 5 years ago

@danfox01 confirmed in prod. Thanks!