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

Add basic index route #179

Merged
merged 8 commits into from
Jun 28, 2023
Merged

Conversation

kylegunby
Copy link
Contributor

@kylegunby kylegunby commented Jun 22, 2023

Ticket

Resolves #177

Changes

Added and index route with basic html including a link to the swagger docs endpoint

Context for reviewers

Previously the index route returned a 404 causing issues with e2e tests.

Testing

Added app/tests/src/route/test_index_route.py

Screenshots

Screenshot 2023-06-22 at 1 48 43 PM

@kylegunby kylegunby requested a review from lorenyu June 22, 2023 16:45
Copy link
Contributor

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

Great work! But I don't think testing should be "N/A" here. How did you know this endpoint works? Can you add a screenshot of what the page looks like when you view it running on localhost?

app/src/app.py Outdated Show resolved Hide resolved
responses:
'200':
content:
application/json:
Copy link
Contributor

Choose a reason for hiding this comment

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

weird, why did apiflask generate this as application/json? is it a bug/unsupported functionality in apiflask or did we need to configure something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw this and was wondering the same thing. I'll take a look at the documentation and see why it did that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having looked at how the generation works before, you'll need to look in apispec: https://apispec.readthedocs.io/en/latest/ as it is what generates the schema

Alternatively, do you think it would make sense to not include this in the schema? I'm not sure about when you define a route directly, but if you make an APIBlueprint, you should be able to pass in enable_openapi=False and it shouldn't be a part of Openapi.

@kylegunby
Copy link
Contributor Author

Great work! But I don't think testing should be "N/A" here. How did you know this endpoint works? Can you add a screenshot of what the page looks like when you view it running on localhost?

That's a good point. I will add a screenshot. I can also add a quick test in tests/src/routes to be safe.

@kylegunby kylegunby requested a review from lorenyu June 22, 2023 20:52
responses:
'200':
content:
application/json:
Copy link
Contributor

Choose a reason for hiding this comment

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

Having looked at how the generation works before, you'll need to look in apispec: https://apispec.readthedocs.io/en/latest/ as it is what generates the schema

Alternatively, do you think it would make sense to not include this in the schema? I'm not sure about when you define a route directly, but if you make an APIBlueprint, you should be able to pass in enable_openapi=False and it shouldn't be a part of Openapi.

@@ -0,0 +1,3 @@
def test_get_index_200(client):
response = client.get("/")
assert response.status_code == 200
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a quick check that the HTML is right? At least that the response body has some of the text?

@lorenyu
Copy link
Contributor

lorenyu commented Jun 28, 2023

Alternatively, do you think it would make sense to not include this in the schema?

i support excluding this endpoint from the schema if it's easy to do. i really don't care what this endpoint returns, just that it's a 200, since the infra e2e tests assume that the index page returns something

@kylegunby kylegunby merged commit 2eff32b into main Jun 28, 2023
2 checks passed
@kylegunby kylegunby deleted the kylegunby/add_response_to_index_endpoint branch June 28, 2023 19:55
kylegunby added a commit to navapbc/platform-test-flask that referenced this pull request Jun 28, 2023
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.

Make site index ("/") return something
4 participants