-
Notifications
You must be signed in to change notification settings - Fork 4
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
Renamed project
filter query parameter to project_id
for Project Summary entities
#668
Conversation
✅ Deploy Preview for ami-dev canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic! Great job finding the appropriate place for these changes and thank you for all of the tests.
Do you feel up to making the changes on the frontend as well? Perhaps doing a pair session with Anna? If we merge it before the frontend is updated, then we have to make the change backwards compatible, which is cool but adds some mess to clean up later.
ami/main/api/views.py
Outdated
@@ -647,6 +653,19 @@ def remove(self, request, pk=None): | |||
} | |||
) | |||
|
|||
@extend_schema( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome that you discovered how to customize the auto-generated docs! I will be copying this approach to other places.
Done with the frontend part! @mihow Should we identify all the places in the backend code where we should change the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I like the reusable function. I think we can use it for the next PR when we start adding permission checks! I just requested one change about cleaning up the occurrences__project
parameter.
@@ -549,7 +549,7 @@ def setUp(self) -> None: | |||
def test_occurrences_for_project(self): | |||
# Test that occurrences are specific to each project | |||
for project in [self.project_one, self.project_two]: | |||
response = self.client.get(f"/api/v2/occurrences/?project={project.pk}") | |||
response = self.client.get(f"/api/v2/occurrences/?project_id={project.pk}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work tracking down all the existing uses!!!
@@ -503,7 +503,7 @@ def get_occurrence_images(self, obj): | |||
|
|||
# request = self.context.get("request") | |||
# project_id = request.query_params.get("project") if request else None | |||
project_id = self.context["request"].query_params["project"] | |||
project_id = self.context["request"].query_params["project_id"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this. When we optimize the Taxa view, I think we will be able to remove this function.
|
||
if not project_id: | ||
if not project: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohamedelabbas1996 in #402 I think we will require a project for all calls. Let's talk about this. We could require the project ID in the URL.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Closes #660