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.

Sign up for GitHub

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

Jump to bottom

Response refactor #39

Merged
merged 25 commits into from Oct 21, 2021
Merged

Response refactor #39

merged 25 commits into from Oct 21, 2021

Conversation

tvaroglu
Copy link
Collaborator

@tvaroglu tvaroglu commented Oct 21, 2021 •
edited

  • Changes Implemented:

    • Add endpoint PATCH /api/v1/stories/<int:pk>
    • Remove PUT from CORS_ALLOWED_METHODS in settings.py (only doing partial updates from FE for name, title, and message, and do not need a PUT request to update all attributes)
    • Refactor JSON responses for all deployed endpoints to adhere to 1.0 Spec
    • Backfill missing request tests for deployed endpoints, happy and sad path
    • Add function to capture lat/lon validation within POST /api/v1/stories endpoint, to ensure database is clear of stories with invalid coordinates
    • Refactor function call to validate coordinates for GET /stories?latitude=<lat>&longitude=<lon> to use same function call and DRY up code
    • Update Postman collection and environment variables now that DB is reseeded (pk's were reset so the GET request for a single story would produce a 404; already pre-wrote the env var for Prod, so this will all still pass post-merge)
    • Update Circle build commands to run pytest --cov for that fancy schmancy coverage report that is so uber-satisfying to look at

  • Quality Control Checklist:

    • >= 20% test coverage
      • 99% yo!!!
    • Postman collection updated (if any changes to endpoints)
    • Last Commit passes CircleCI build

  • Blockers (if applicable): None

  • Next Steps & Additional Notes:

    • Wrap up GeoLoc logic for index and show endpoints

matt-kragen and others added 17 commits October 18, 2021 20:51
@matt-kragen
Begin TDD for serializer
9235660
@matt-kragen
Begin function for 'attributes'
d7608e7
@tvaroglu
Add sserializer test for reformat function
a10d496
@tvaroglu
Refactor StorySerializer.reformat()
ff740cd
@tvaroglu
Add working request spec for GET /stories/<int:pk> based on refactore…
07d7668 
…d JSON response
@tvaroglu
Update postman collection
57700e7
@tvaroglu
Complete request_test happy and sad path for POST request
3ee9aac
@tvaroglu
Update Postman collection
ee20cdb
@tvaroglu
Add an implementation for POST /stories with custom lat/long validation
c963adf
@tvaroglu
Refactor coordinate validation for lat/lon
b4ea71f
@tvaroglu
Update Postman environments and test collection based on response and…
643a93f 
… error handling refactors
@tvaroglu
Add request test for deletion of existing story
8376264
@tvaroglu
Add refactored JSON response for PATCH request
0e93493
@tvaroglu
Refactor test_requests
297cf6e
@tvaroglu
Add PATCH request to Postman collection
deed684
@tvaroglu
Add additional edge case test to test requests and Postman suite
8c7e366
@tvaroglu
Remove PUT from CORS_ALLOWED_METHODS
9c1c974
@tvaroglu tvaroglu self-assigned this Oct 21, 2021
@tvaroglu tvaroglu added this to In-Progress in MIB via automation Oct 21, 2021
This was linked to issues Oct 21, 2021
PATCH /api/v1/stories/<int:pk> #21
Closed
Refactor JSON responses for deployed endpoints #25
Closed
This was linked to issues Oct 21, 2021
Backfill Pytest request tests for deployed endpoints #26
Closed
Validate Longitude and Latitude #34
Closed
@tvaroglu tvaroglu moved this from In-Progress to Ready for Review in MIB Oct 21, 2021
@tvaroglu tvaroglu added bug Something isn't working Database labels Oct 21, 2021
@marlitas
Copy link
Collaborator

Taylor, this might be one where rebasing the docs branch would be most appropriate. I can try to rebase it on my local machine and let you know what happens.

marlitas
marlitas approved these changes Oct 21, 2021
View reviewed changes
Copy link
Collaborator

@marlitas marlitas left a comment

Choose a reason for hiding this comment

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

Looks really good. I'll make some changes to my PR to be a little more compatible with this.

message_in_a_bottle/api/models.py Outdated
else:
return False

# TODO: rename this function to be more specific to MapQuest Service
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. I can add that to my error handling branch/PR since it is already dealing with this function.

message_in_a_bottle/api/tests/test_requests.py
@@ -6,15 +6,15 @@
from message_in_a_bottle.api.serializers import StorySerializer

@pytest.mark.django_db
class TestGetStory(TestCase):
class TestStoryRequests(TestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're probably gonna get some merge conflicts with this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed - I would propose the error_handling PR (I am assuming that is the one we should refer to, as it looks much more complete / less commented-out code?) should just include tests / changes for the GET (index) endpoint and associated tests. I can back into new test structure from there from all other CRUD I was working on this branch.

MIB automation moved this from Ready for Review to In-Progress Oct 21, 2021
@tvaroglu tvaroglu marked this pull request as draft October 21, 2021 21:27
@tvaroglu tvaroglu marked this pull request as ready for review October 21, 2021 22:44
@matt-kragen matt-kragen self-requested a review October 21, 2021 22:45
matt-kragen
matt-kragen approved these changes Oct 21, 2021
View reviewed changes
Copy link
Collaborator

@matt-kragen matt-kragen left a comment

Choose a reason for hiding this comment

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

Lots of intuitive changes! Glad to see that our coverage is still that high.

@matt-kragen matt-kragen merged commit 0292607 into main Oct 21, 2021
MIB automation moved this from In-Progress to Done (Deployed) Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marlitas marlitas marlitas approved these changes

@matt-kragen matt-kragen matt-kragen approved these changes

Assignees

@tvaroglu tvaroglu

Labels
Architecture Back-End bug Something isn't working Database DevOps Endpoint
Projects
MIB
Done (Deployed)
Milestone
No milestone
3 participants
@tvaroglu @marlitas @matt-kragen