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

implementing org apiview GET method #1159

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

to-sta
Copy link
Collaborator

@to-sta to-sta commented Mar 15, 2025

Contributor checklist


Description

Implemented a paginated org APIView for the organizations endpoint and added testing for the endpoint.

Copy link

netlify bot commented Mar 15, 2025

Deploy Preview for activist-org canceled.

Name Link
🔨 Latest commit be1583b
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/67dfce358b88220008b0da6b

Copy link
Contributor

Thank you for the pull request! ❤️

The activist team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider attending our bi-weekly Saturday developer syncs! It'd be great to meet you 😊

Copy link
Contributor

Maintainer Checklist

The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

  • The TypeScript, pytest and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • The changelog has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis
Copy link
Member

Not sure on the social link test in the backend, @to-sta, but for the license header check just add the one liner that you see on the top of most files to the ones that are missing it 😊

Thanks for opening this!

@mattburnett-repo
Copy link
Member

@andrewtavis @to-sta Saw something about the tests for social links; anything I can explain / help with?

@andrewtavis
Copy link
Member

Ya I was going to ping you on this, @mattburnett-repo. Do you have an idea on why the changes would be causing them to fail?

Line is:


FAILED backend/content/tests/test_social_links.py::test_organization_social_link_list_view - assert 404 == 200

@mattburnett-repo
Copy link
Member

mattburnett-repo commented Mar 15, 2025

The test is simple and basic. Here's the line we care about:

response = client.get(f"/v1/communities/organizations/{org.id}/")

This should just pull a single big org object, which has social link info as part of the composite object. The test then just looks at the attached soc link info. Really simple, but the test fails before this part.

So a 404 means, for the get request, that either there is no more GET (list) method, the url part is broken, the org.id part is broken, or the endpoint just isn't responding correctly for whatever reason.

===

I noticed in the backend/communities/urls.py that is part of this PR, that lines 25-27

router.register(
prefix=r"organizations", viewset=OrganizationViewSet, basename="organization"
)

have been removed and replaced with lines 58-60

urlpatterns = [
path("", include(router.urls)),
path("organizations/", OrganizationAPIView.as_view()),
]

It looks like the only real difference is that there are two different Views/ViewSets, and the path/prefix thing ('organizations') now ends with a '/'. Beyond that, I don't know anything about the new code.

So maybe this is the place to start, to figure out what the solution should be? I don't know what the intent of the new code is, so I can't say anything about it. One guess is that the '.../organizations/' url doesn't take an org id at the end of the path anymore?

===

If the new code / endpoint in the PR is what we want, we could simply adjust the test to fit the newer code. That would probably be a simple matter of changing the URL that the test uses.

@to-sta
Copy link
Collaborator Author

to-sta commented Mar 15, 2025

@mattburnett-repo maybe I should added a WIP in the description 😄. Finished adding the GET (list) and POST method but the detail view is not implemented yet. Therefore your test is failing.

@mattburnett-repo
Copy link
Member

Makes total sense, @to-sta! Thanks for the detail info :)

@to-sta
Copy link
Collaborator Author

to-sta commented Mar 15, 2025

I recommend changing the status field from a ForeignKey to a ChoiceField. The current implementation is overly complex and causes issues with pytest during teardown—likely a classic 'chicken-and-egg' problem 😄 . Ultimately, I resolved the issue by removing the default values.

@andrewtavis
Copy link
Member

Thanks for the explanation and checking in here, @to-sta and @mattburnett-repo :) Requested reviews for when this is ready. Please let us know when it's ready, @to-sta! 😊

@mattburnett-repo mattburnett-repo self-requested a review March 16, 2025 16:44
@mattburnett-repo
Copy link
Member

Re-requested review because I may have approved too early. Let me know when we're ready for review :)

@to-sta
Copy link
Collaborator Author

to-sta commented Mar 18, 2025

Ok 😕. Locally the test are running just fine:

communities/organizations/tests/test_api.py::test_organizationDetaiAPIView PASSED                                                               [ 40%]
communities/organizations/tests/test_org_events.py::test_multiple_events_per_org PASSED                                                         [ 42%]

But during the GitHub workflow django is not able to load the fixtures:

FAILED backend/communities/organizations/tests/test_api.py::test_organizationDetaiAPIView - communities.models.StatusType.DoesNotExist: StatusType matching query does not exist.
ERROR backend/communities/organizations/tests/test_api.py::test_OrganizationAPIView - django.core.management.base.CommandError: No fixture named 'status_types' found.

@to-sta
Copy link
Collaborator Author

to-sta commented Mar 18, 2025

Ok, got it fixed with using the pathlib libary to get the fixture folder location.

Almost done with the PR

  • GET (List)
  • GET (Detail)
  • POST
  • DELETE
  • PUT

@to-sta
Copy link
Collaborator Author

to-sta commented Mar 19, 2025

Implemented Final Method & Extended API Documentation.

In this update, I implemented the last remaining method and extended the API documentation. However, I encountered an issue caused by the djangorestframework-camel-case package in combination with drf-spectacular.

Issue Description

Requests made via SwaggerUI are incorrectly transformed. For example:

Expected URL:

http://localhost:8000/v1/communities/organizations/3fa85f64-5717-4562-b3fc-2c963f66afa6/

Actual URL:

http://localhost:8000/v1/communities/organizations/%7Borg_id%7D/

Debugging Attempts

Suspecting the issue might be related to the camelCase parser, I tested my theory by temporarily disabling it. I commented out the following lines in the settings:

"DEFAULT_PARSER_CLASSES": (
    "djangorestframework_camel_case.parser.CamelCaseFormParser",
    "djangorestframework_camel_case.parser.CamelCaseMultiPartParser",
    "djangorestframework_camel_case.parser.CamelCaseJSONParser",
),

...

"CAMELIZE_NAMES": False,
"POSTPROCESSING_HOOKS": [
    "drf_spectacular.hooks.postprocess_schema_enums",
    "drf_spectacular.contrib.djangorestframework_camel_case.camelize_serializer_fields",
],

And it worked properly again. Any insights or suggestions on how to solve this are welcome!

@to-sta
Copy link
Collaborator Author

to-sta commented Mar 23, 2025

I found a workaround. As long as we do not use snake_case in url parameters the transformation is as expected..

urlpatterns = [
    path("", include(router.urls)),
    path("organizations/", OrganizationAPIView.as_view()),
    # old
    path("organizations/<uuid:ord_id>/", OrganizationDetailAPIView.as_view()),
    # new
     path("organizations/<uuid:id>/", OrganizationDetailAPIView.as_view()),
]

This was referenced Mar 23, 2025
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.

3 participants