Cyberstorm API: return categories and sections for a team #895
Conversation
It felt a bit dirty to import such a simple serializer from another app, especially since that serializer is marked experimental. Refs TS-1860
If community is defined, return a list of community's PackageCategories and PackageListingSections too. This is done via optional query parameter so both views that have access to community (e.g. team's package listing view) and don't have it (e.g. team's profile settings view) can use the same endpoint. Refs TS-1856
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## cyberstorm-team #895 +/- ##
===================================================
+ Coverage 92.27% 92.31% +0.03%
===================================================
Files 271 272 +1
Lines 7820 7842 +22
Branches 743 745 +2
===================================================
+ Hits 7216 7239 +23
+ Misses 502 501 -1
Partials 102 102
☔ View full report in Codecov by Sentry. |
| sections = serializers.SerializerMethodField() | ||
|
|
||
| def get_package_categories(self, obj): | ||
| community_id = self.context["request"].GET.get("community") |
There was a problem hiding this comment.
I don't like the concept of having serializers depend on a request object. That's caused issues in the past and probably should be good to avoid in the future too. I'd go as far as saying using serializer context is a bad practice in general because of how easily it turns into a footgun.
I also feel like package_categories and sections don't really have a purpose existing under the serializer of a Team object. If they need to be bundled in to a response together, that seems like something the view class should handle sorting out?
I'm assuming these exist here to provide functionality for the team-filtered package search view. Perhaps editing the package search API view to take a team ID optional parameter would be a better approach to achieve that? That is after all already implicitly community-scoped, which would make this a lot more straight forward.
There was a problem hiding this comment.
I don't like the concept of having serializers depend on a request object. That's caused issues in the past and probably should be good to avoid in the future too. I'd go as far as saying using serializer context is a bad practice in general because of how easily it turns into a footgun.
I also feel like
package_categoriesandsectionsdon't really have a purpose existing under the serializer of aTeamobject. If they need to be bundled in to a response together, that seems like something the view class should handle sorting out?
I've no strong feelings regarding this part, but it might be a bit irrelevant in this particular case, giving the rest of the reply.
I'm assuming these exist here to provide functionality for the team-filtered package search view.
Yes, this is due to trying to reuse the endpoint for both team-filtered package search view, and the main tab of team's settings view.
Perhaps editing the package search API view to take a team ID optional parameter would be a better approach to achieve that? That is after all already implicitly community-scoped, which would make this a lot more straight forward.
I don't like this idea. Currently I have the client side designed to have three layers:
-
Layout components (currently one for a community and one for a team, in the future probably one for user too). This fetches package categories and sections from backend. The rationale being that they're static in the sense they only depend on the community (currently), so fetch them once on the top level to keep the other layers simpler. Doesn't have internal state.
-
PackageSearch component, used by all the layout components. Handles the client side state: what filters are selected, what's written in the text search input. Doesn't communicate with the backend at all.
-
PackageList component, used by PackageSearch component. Handless fetching the packages from the backend, mostly based on the props passed by PackageSearch. Doesn't fetch categories/sections, because filters don't affect them, e.g. refetching them on every change on the text search is unnecessary. I would have liked this component to have no internal state, but a) it handles the state for pagination, since pagination information is returned by backend when packages themselves are fetched, and b) it handles the state of ordering the results, just for UI layout reasons.
So, as a compromise, would it be better to have a separate endpoint for fetching the categories/sections? That way they wouldn't affect the serializers, and wouldn't mess with the design I've chosen.
There was a problem hiding this comment.
So, as a compromise, would it be better to have a separate endpoint for fetching the categories/sections? That way they wouldn't affect the serializers, and wouldn't mess with the design I've chosen.
This sounds like it would address my primary concern, which is that of mixing data unrelated to teams in the team serializer. Also I want to clarify I'm not against having view-specific serializers (e.g. FooApiResponseSerializer is a pattern used in a few) that contain multiple different objects nested inside, the main part I didn't like here is how the serializer & view presented themselves as being bound to a specific object type in a CRUD like fashion, but still returned (arguably unrelated) data beyond that scope entirely.
|
Closing and reimplementing as a separate endpoint. |
Cyberstorm API: add CyberstormPackageCategorySerializer
It felt a bit dirty to import such a simple serializer from another
app, especially since that serializer is marked experimental.
Refs TS-1860
Cyberstorm API: return team's categories and sections
If community is defined, return a list of community's PackageCategories
and PackageListingSections too. This is done via optional query
parameter so both views that have access to community (e.g. team's
package listing view) and don't have it (e.g. team's profile settings
view) can use the same endpoint.
Refs TS-1856