search

indiehd / web-api

GNU Affero General Public License v3.0
6 stars 4 forks source link

Question: what does catalogable() on user do? #174

Closed mblarsen closed 4 years ago

mblarsen commented 4 years ago

Actually on UserRepository:

        return $this->user->entity->catalogable ?: $this->user->entity->catalogable;

Isn't this the same as:

        return $this->user->entity->catalogable;
cbj4074 commented 4 years ago

It sure is! I'm not sure if that was stubbed-in as some kind of placeholder, or just an oversight.

@poppabear8883 added that originally, so perhaps he can chime-in.

cbj4074 commented 4 years ago

I took another look at this and that entity() method doesn't even exist on the User model, so I'm not sure what the intention was here.

There is, however, an entities() method (plural), so it seems probable that we changed entity() to entities() at some point, likely upon the realization that it should be possible for a single User to be associated with more than one Catalog Entity (i.e., an Artist or Label).

The tests do not cover this line, as this code was written before we adopted true TDD; if they did, an exception would surely be thrown.

If I had to guess, the intention here was probably to return the Catalog Entity with which the user is associated, if any. Maybe the the right side of the expression was meant to be null.

Examining the songs() method in that same UserRepository class may also shed some light on the intention:

https://github.com/indiehd/web-api/blob/e8f462cc86b99c671906706721e4ad06d0e8b75b/app/Repositories/UserRepository.php#L61-L64

The logic here seems to be, "If this User is associated with a Catalog Entity (an Artist or a Label), then return the Songs associated with that entity. Otherwise, return the Songs that the User has purchased."

In other words, the songs() method behaves differently, depending on whether the User is effectively selling music vs. buying music.

But now that I take a closer look at this method, it's still not quite right, because it's perfectly plausible that a User who happens to be associated with an Artist entity, for example, could purchase a different Artist's music on the site, in which case that songs() method would never return the User's purchased music (it would always return his own music that he has posted for sale).

So, the long-and-short seems to be that these methods need to be fixed and tested.

mblarsen commented 4 years ago

Covered in https://github.com/indiehd/docs/pull/54