Package listing action views (TS-2297)

[Roffenlund]

This PR introduces serializers and views in the Cyberstorm API for package listing actions: update categories, reject, and approve.

Changes Introduced:

• New serializers for package listing actions.
• New views implementing the actions: update categories, reject, and approve.
• Unit tests to validate the functionality.
• URL configuration updated to include the new views.

The code is based on the existing logic from the experimental API, but it has been heavily refactored. While the underlying functionality remains the same, the implementation is more cleaner.

[MythicManiac]

I haven't spotted anything obviously wrong with the functionality itself yet at least, but I do have thoughts on the coding convention choices. I'll need to process my thoughts a bit more before I can really give feedback on those, but leaving this WIP review for now.

The main thing that stands out to me compared to the experimental API implementation is that there's a lot more jumping around between different functions & abstractions, making it harder to track what's actually going on when reading the code. These most likely aren't something I'll end up considering blockers, but every time I come across such an instance I'm prompted to think about it, making the review take longer.

A simple example to illustrate the above:

class SampleA:

  def bar():
    do_something()

  def foo():
    if some_condition():
      bar()

vs.

class SampleB:

  def foo():
    if some_condition():
      do_something()

Between these two examples, properly reviewing SampleA requires significantly more attention to detail (and memorization of the code) compared to reviewing SampleB, especially if the functions are far apart from each other and the PR is large. Obviously the point here isn't to avoid splitting code to smaller pieces, but rather that there is a cost involved in doing so. The cost is also something not directly apparent to the writer of the code, as the writer can safely make assumptions about the code which the readers can not. Having & enforcing project-wide coding conventions is one way to reduce that cost (as then everyone in the project should be able to make the same assumptions), but in general minimizing the amount of assumptions & required prior knowledge for understanding any piece of code is probably a good default to strive for.

No need to read into this feedback too deeply, just sharing some thoughts I've had while reviewing. A large portion of the time I spend reviewing tends to be taken up by considerations like this & deciding whether or not I want to comment on them, often far surpassing the time spent reviewing the actual functionality being introduced or changed.

[Roffenlund]

I haven't spotted anything obviously wrong with the functionality itself yet at least, but I do have thoughts on the coding convention choices. I'll need to process my thoughts a bit more before I can really give feedback on those, but leaving this WIP review for now.

The main thing that stands out to me compared to the experimental API implementation is that there's a lot more jumping around between different functions & abstractions, making it harder to track what's actually going on when reading the code. These most likely aren't something I'll end up considering blockers, but every time I come across such an instance I'm prompted to think about it, making the review take longer.

A simple example to illustrate the above:

class SampleA:

  def bar():
    do_something()

  def foo():
    if some_condition():
      bar()

vs.

class SampleB:

  def foo():
    if some_condition():
      do_something()

Between these two examples, properly reviewing SampleA requires significantly more attention to detail (and memorization of the code) compared to reviewing SampleB, especially if the functions are far apart from each other and the PR is large. Obviously the point here isn't to avoid splitting code to smaller pieces, but rather that there is a cost involved in doing so. The cost is also something not directly apparent to the writer of the code, as the writer can safely make assumptions about the code which the readers can not. Having & enforcing project-wide coding conventions is one way to reduce that cost (as then everyone in the project should be able to make the same assumptions), but in general minimizing the amount of assumptions & required prior knowledge for understanding any piece of code is probably a good default to strive for.

No need to read into this feedback too deeply, just sharing some thoughts I've had while reviewing. A large portion of the time I spend reviewing tends to be taken up by considerations like this & deciding whether or not I want to comment on them, often far surpassing the time spent reviewing the actual functionality being introduced or changed.

I made some changes that I believe align with your feedback, preserving locality of behavior and clean code practices without introducing excessive duplication or complexity. Please see latest commit.

[x753]

I made some changes that I believe align with your feedback, preserving locality of behavior and clean code practices without introducing excessive duplication or complexity. Please see latest commit.

Look at your ApprovePackageListingAPIView and compare it to PackageListingApproveApiView in api/experimental/views/listing.py. In the old experimental API, everything reads top to bottom. In your cyberstorm API, you either need to remember what _approve does or go back to it when you reach it in post() (the sort of "jump" Mythic was referring to).

Also note that in the experimental API each parameter in listing.approve() is on its own line, rather than assigning variables and writing the function on one line.

a = object.property1
b = object.property2
sample_1(a, b)

vs.

sample_2(
    a=object.property1,
    b=object.property2,
)

sample_2() is easier to review because I don't have to remember what a or b was.

This isn't to say that parameters should always be on their own lines, but it's definitely appropriate here where there are multiple parameters consisting of objects with properties that don't need to be assigned to variables and reused later.

[Roffenlund]

Look at your ApprovePackageListingAPIView and compare it to PackageListingApproveApiView in api/experimental/views/listing.py. In the old experimental API, everything reads top to bottom. In your cyberstorm API, you either need to remember what _approve does or go back to it when you reach it in post() (the sort of "jump" Mythic was referring to).

This is essentially a trade-off between cleaner code and maintaining the locality of behavior. I extracted functionality that didn’t belong in the post() function into a separate function while moving relevant functionality back into the post() function, which was previously delegated to its own function. From my perspective, this approach preserves the locality of behavior without adding unnecessary complexity.

This isn't to say that parameters should always be on their own lines, but it's definitely appropriate here where there are multiple parameters consisting of objects with properties that don't need to be assigned to variables and reused later.

In some cases, I would agree, but not in this one for several reasons. If the actual code was like sample_2 in your example, I would leave it as is. However, it looks like this:

listing.approve(
    agent=request.user,
    internal_notes=params.get("internal_notes"),
)

This inline expression isn’t bad, but in general, I believe variables should be assigned on a separate line when derived from a function call. In this case, assigning the variables first makes the code easier to read and potentially easier to debug. However, I would decide this on a case-by-case basis, as there isn’t necessarily a right or wrong approach here.