[Feature] Featured Artists

[poppabear8883]

Preface:

Even though "Feature", in the context of a polymorphic, could probably be spelled "FeaturableorFeatureable` (even though neither is a real word), the former is more consistent with relevant comparisons such as "Configure" becoming "Configurable" in English (i.e., it's not "Configureable").

Also, with respect to Laravel's inbuilt conventions, a Feature demands a table named featureds (as opposed to features), as awkward as it may seem. It is preferable to satisfy the convention vs. specifying a custom table name property in the model definition.

TDD

• [x] Write Tests based on below algorithm

Some of the algorithm that I have in mind thus far: This is just an initial thought, will discuss further

• An artist MUST have a completed Account Profile (not necessary because a profile is compulsory when creating an account in the first place)

• An artist MUST be featured for 7 days before new results are queried

  • Checks featured table created_at field
  • If 7 days has elapsed, query another artist
  • *Create a featured artist; new Featured Artists are pushed onto the table, and any given Artist can have any number of Features (provides for logging and Featured history), but only the newest one is ever retrieved

• An artist MUST have 1 or more ACTIVE albums

• An artist MUST have a total of 5 or more songs (this would prevent a break-out single from being featured, for example, which we don't want to do)

• An artist MUST have $50 or more sold in the past 15 days (on-hold, until we implement Checkout, as we have no means by which to make this determination currently; created #138 to track this)

  • Trending

• An artist MUST NOT be featured again for at least 180 Days

  • BEFORE Create, check if the artist exists in featured table
  • If not, continue to create and feature said artist
  • If so, check updated_at to see if 180 days has elapsed
  • If so, continue to create and feature said artist

DB Setup

• [x] Featured DB Migration History of featured entities

  table: 'featureds',
  fields: {
     id: 'PK AutoIncr',
     featurable_id: 'Int',
     featurable_type: 'String',
     created_at: 'Carbon Timestamp',
     updated_at: 'Carbon Timestamp'
  }

• [x] Seeds for test data

Api Implementation

• [x] Api Resource

  {
  moniker: 'String',
  songs_count: 'Int',
  albums_count: 'Int',
  profile_url: 'String'
  }

• [x] FeaturedRepository
• [x] FeaturedController index() only Limit 3 results (currently, there is no means by which to limit the number of results, and #139 has been created to track that effort)
• [x] API Route, GET route only

Edit this as necessary

[cbj4074]

This all looks pretty reasonable to me; nicely done, and thanks for fleshing it out.

The only thought I have is that we shouldn't require the Artist to have more than one song, because it's entirely possible that an artist has a break-out single that generates significant traction.

In the case of a single, the artist would have one album with one song, so as long as one of each is required, the relevant criteria would be met.

Other than that one change, this is a great starting-point.

[cbj4074]

I thought I'd share a handful of observations made while writing the first couple of tests.

As discussed in a side-channel via real-time chat, these tests can be categorized by those that apply to "eligibility" (that is, determining which Artists are eligible to be Featured), and those that apply to retrieving Artists that are currently Featured.

With regard to eligibility, the following Artist records can be used to test all required scenarios:

Artists who do not meet the eligibility requirements:

• Artist doesn't have a Profile
• Artist doesn't have at least one Album
• Artist has a Profile and at least one Album, but has already been Featured within the past 180 days

Artists who do meet the eligibility requirements:

• Artist has a Profile
• Artist has at least one Album
• Artist has a Profile and at least one Album, and has been Featured within the past 180 days, but has been Featured fewer than 7 days.

It seems that these requirements can be tested in a single assertion, which looks something like this:

public function testFeaturableResultsMatchExpected()
{
    $this->assertEquals(
        collect($this->eligibleArtists)->pluck('id'),
        $this->artist->featurable()->get()->pluck('id')
    );
}

The Artists that meet the eligibility criteria are captured during setUp(), and stored in $this->eligibleArtists. This simple test ensures that only the two Artists that are expected to be eligible are returned when querying all Artists that are eligible to be Featured.

The remaining tests, focused around retrieval of Artists who are currently Featured, might be as simple as the following:

• testFeaturedEntityIsInstanceOfArtist()

Finally, I'm not sure that the updated_at value needs to be taken into consideration. In fact, I'm not sure that we should alter existing rows at all. Instead, it might be better to insert a new row for each instance in which an Artist is Featured, in which case an Artist could have any number of rows in the featureds table, but only one would be considered upon retrieval.

This has several benefits, not least of which is that an Artist has insight into how many times it was featured over time, which has value. This approach would also simplify the logic in that the is_active flag could be eliminated entirely in favor of always selecting the newest record for a given Artist.

[poppabear8883]

Artist has a Profile and at least one Album, and has NOT been Featured within the past 180 days OR has NOT been featured at all (no record in featureds table ?

[poppabear8883]

Finally, I'm not sure that the updated_at value needs to be taken into consideration.

The idea behind this is that when a featured artist has been created, its updated_at field matches that of the created_at field ... on creation of a featured entity the default value for active is set to true, after 7 days of being active, the system will update the records active field to false which will then update the updated_at field giving the start date/time for the 180 days since the entity has been featured ?

[cbj4074]

and has NOT been Featured within the past 180 days OR has NOT been featured at all (no record in featureds table)?

If I'm not mistaken, this satisfies both requirements:

public function scopeFeaturable($query)
{
    return $query->has('profile')
        ->whereHas('albums', function ($query) {
            $query->where('is_active', 1);
        })
        ->whereDoesntHave('featureds', function ($query) {
            // "... where created_at is more recent than 180 days ago."
            // The goal is to exclude Artists who have been Featured in the
            // past 180 days.

            $query->where('created_at', '>', Carbon::now()->subDays(180)->toDateTimeString());
        });
}

https://github.com/indiehd/web-api/pull/137/files/dc7b058ea3d43bd03654e0eda08f070f2af2bd0c..2ccaecc8dea3f69b7df81daa7222fe3ebe697fa0#diff-73e58b8f814f3ef0a5f1447684536909R49

In other words, "where Artist doesn't have a Feature that is more recent than 180 days" implies "where artist doesn't have a feature at all". There's no need to make any further distinction, is there?

[poppabear8883]

how many times it was featured over time, which has value.

I wonder if this can be wrapped into a Activity Log feature

Every account has an ActivityLog and an ActivityLog belongs to an account ?

[cbj4074]

I wonder if this can be wrapped into a Activity Log feature

Sure, that is certainly something to consider. Let's table it for discussion, and consider implementing it after the MVP.

[cbj4074]

The idea behind this is that when a featured artist has been created, its updated_at field matches that of the created_at field ...

Indeed, which does make sense. I'm just wondering if the logic can be simplified such that we can both eliminate the need to maintain the is_active field (which, by extension, would eliminate the need to take the updated_at value into consideration) and store the full history of how many times a given Artist has been Featured.

Let me think through this a bit more.

[cbj4074]

My thinking in this regard is that the mechanism that determines eligibility to be Featured should be completely separate from the mechanism that stores and retrieves Featured entities (the FeaturedRepository).

Where the storage mechanism is concerned, this means, in effect, "I simply store whichever entities I'm told are eligible."

With this approach, storing the is_active value, and manipulating it as described in a previous comment, becomes unnecessary.

That said, the real value in storing is_active derives from using it in combination with the featurable_type column to create a UNIQUE constraint, which would prevent the FeaturedRespository from storing a duplicate entry, i.e., "trying to feature an Artist who is already featured". But that's the thing: as long as the eligibility logic is sound, that possibility should never arise. So, while storing is_active does provide somewhat of a safeguard in this respect, maintaining that column's value introduces complexity that, in my opinion, outweighs the benefit of this safeguard.

And furthermore, what's the worst that would happen if a duplicate row were to be inserted? The FeaturedRepository's retrieval method would effectively order by created_at desc limit 1, so inserting a duplicate row would in essence simply "renew the affected Artist's feature". Certainly not ideal, but, again, it should never happen in the first place.

Finally, the updated_at value (and by extension, the is_active value) isn't necessary for calculating the maximum feature period (180 days, for example); it can be calculated from created_at.

If anything I've stated is incorrect, or if I've overlooked anything, by all means correct me.

Edit to Add:

Without the is_active flag, we have no means by which to avoid showing an expired Feature, unless we consider the created_at value when querying the Featured entities.

Also, to ensure that only a given entity's most recent Feature of any given type is returned, it would be necessary to ORDER BY created_at DESC GROUP BY [all columns in SELECT] LIMIT 1, effectively.

[cbj4074]

In consideration of my most recent commit to the associated PR, the only question I have is in relation to limiting the results that the API endpoint returns to 3 records.

Where should this limit be enforced? Given the current implementation, I don't see any way to limit the number of results. The relevant code looks to be here:

https://github.com/indiehd/web-api/blob/e5bec4fffa4238afaa6e4ddc2f128279c940191d/app/Http/Controllers/Api/ApiController.php#L64

For what it's worth, given the current configuration, only 3 Artists are Featured at any given time, so only 3 records will be returned anyway, but I figured I'd inquire in this regard nonetheless.

[cbj4074]

Also, I created #138 so we don't forget to implement the Trending eligibility requirement.