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

Updated Swagger #506

Merged
merged 6 commits into from
Mar 16, 2019
Merged

Updated Swagger #506

merged 6 commits into from
Mar 16, 2019

Conversation

btyy77c
Copy link
Contributor

@btyy77c btyy77c commented Oct 7, 2018

Context

  • Installed Swagger using webpack and removed the html/js/css from the public folder.

Summary of Changes

  • Deleted files from the public folder
  • Install Swagger using Yarn
  • Added Route, Controller, View, and JS to run Swagger

WARNING: Since I updated swagger, the classes changed and the css is no longer valid. I added some basic CSS but the swagger layout is completely different now.

Checklist

  • Tested Mobile Responsiveness
  • Added Unit Tests
  • CI Passes
  • Deploys to Heroku on test Correctly (Maintainers will handle)
  • Added Documentation (Service and Code when required)

Screenshots

Before

After

@DeeDeeG DeeDeeG added Ready for Review Hacktoberfest These are issues or pull requests related to Hacktoberfest (https://hacktoberfest.digitalocean.com/) packages labels Oct 7, 2018
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 7, 2018

@btyy77c, Thanks for this! I'll test this in just a moment and see if this works on my machine.

@DeeDeeG

This comment has been minimized.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 7, 2018

This looks pretty good!

Link to screenshot.

The CSS does need a bit of updating, sure, but it's functional.

Unrelatedly to this pull request, my Docker instance still isn't loading any restroom entries from db/export.csv, so this is hard to test unless you manually submit a new restroom during testing.
Update: see comment below.

I'm not sure if we should allow the POST method at this time, would want feedback from @mi-wood or @tkwidmer. But if we get API submission functionality for free by doing that, we would eventually want to enable it (after making sure we have good spam filtering I suppose.)

All-around, my impression is that it's better to have this as a nodejs dependency than pasted into the repo as html/css/js files. So I'd personally be happy to merge once we're finished reviewing and perhaps polishing the appearance and/or tweaking the configs a bit.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 8, 2018

I have a pull request that makes it easier to test this sort of thing: #508

@tkwidmer
Copy link
Contributor

tkwidmer commented Oct 8, 2018

Overall this looks really great to me. My only concern is adding a POST, although this is something we obviously want to do. I don't want to end up polluting the db with a lot of random requests.

This was referenced Oct 8, 2018
@btyy77c
Copy link
Contributor Author

btyy77c commented Oct 10, 2018

Overall this looks really great to me. My only concern is adding a POST, although this is something we obviously want to do. I don't want to end up polluting the db with a lot of random requests.

Can you help me understand where you are seeing the new POST action? No POST action was added from this branch. It's possible it's showing you a POST action already available in production, but I'm not seeing any POSTs when I view the page. Thanks!

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 10, 2018

My apologies for the mixup, I can see now that POST is not functionally new in this pull request.

Can you help me understand where you are seeing the new POST action?

When you submit an API request through the docs page, the new interface exposes what methods are allowed:

new interface showing allowed methods, including GET POST and OPTIONS

(The old interface didn't show that info.)

No POST action was added from this branch. It's possible it's showing you a POST action already available in production [. . .] .

Yep. That's what happened. (Whoops.)

Conclusion: If we want to discuss or address the fact we appear to (already, regardless of this pull request) allow POST through the API, I think we can move that discussion out of this pull request.

I'd also like to do some work on the CSS, and maybe even consider the older version of the package; This isn't powering the underlying API backend, just the documentation interface (front end). If the older package was more user-friendly, we may as well go with that, barring any security patches that may have been added. Will look into this when I get time. (I'm sort of taking the day off, but wanted to respond to this.)

Thanks for pointing that out.

Best,

-DeeDeeG

@DeeDeeG

This comment has been minimized.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 16, 2018

For reference, swagger-ui and swagger-ui-dist with no custom CSS both look exactly like this:

Swagger UI API documentation with default, bare-bones placeholder styling

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Nov 24, 2018

There is actually a stylesheet included in the node package swagger-ui.

See: https://github.com/swagger-api/swagger-ui/blob/master/dist/swagger-ui.css

I'm really unfamiliar with JavaScript programming, and with the Rails asset pipeline, (and with Webpacker...) so it's unclear to me how to use this.

I actually prefer what this Pull Request has so far over that stylesheet, though. So maybe we can update the stylesheet slightly so it looks more like what we have in production. (Or if someone understands better how this all works, we might be able to use the swagger-ui package's 2.x series.)

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Nov 26, 2018

2.x branch of swagger-ui can't be used since it does not support recent Grape versions. We are looking to update Grape to at least 1.1 when we get the chance, so I'm thinking this pull request is the right time to do that.

@DeeDeeG DeeDeeG force-pushed the docsPage branch 2 times, most recently from 2e9c676 to 839d701 Compare November 26, 2018 23:58
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Dec 9, 2018

I think we can/should merge this as-is and do any fixups we need later. It would be nice to have up-to-date Grape and Swagger-UI.

(CSS tweaks or tweaks to the API code could come later if we want. But neither of those is strictly needed, they're just nice-to-haves.)

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Mar 12, 2019

Would like to finally merge this soon.

I have a work-in-progress branch that fixes up the CSS foor the API docs page, done so as to mostly match the visual style of the 2.x branch of swagger-ui.

But it would be great to have this merged. Particularly since it upgrades grape.


Would want to co-ordinate with mobile app folks on this. Want to make sure this will not change the .json output of the API. We don't want to go breaking the apps!

I already know this would have the exact same formatting as it does now: https://refugerestrooms.org/api/v1/restrooms.json

But this file would change a bunch: https://refugerestrooms.org/api/swagger_doc.json (becomes much longer and more detailed, none of the fields seem to stay the same.)

URLs are stable from before this update to after, so there shouldn't be a need to update request URLs or anything.

(cc @hissingpanda, @Carmen-Git-It @hkellaway )

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Mar 16, 2019

Merging so we can test the mobile apps against the develop branch's (auto-updating) Heroku instance: https://refuge-staging-14.herokuapp.com/

(Once I update this branch (rebase it on top of develop), and assuming that passes CI)

Copy link
Contributor

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Thanks, and sorry it took so long!


This updates our grape gem (API backend framework) and swagger-ui (API documentation and demonstration UI).

Looks good to me.

@DeeDeeG DeeDeeG merged commit 356e72a into RefugeRestrooms:develop Mar 16, 2019
@btyy77c btyy77c deleted the docsPage branch December 16, 2019 11:44
@DeeDeeG DeeDeeG mentioned this pull request Dec 16, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Hacktoberfest These are issues or pull requests related to Hacktoberfest (https://hacktoberfest.digitalocean.com/) packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants