Next

I refactored the feed functionality out of the API request code and into Core\Boost\Feed abstract class and implemented Core\Boost\Feeds\Boost concrete class. In the review site the feed request response exactly matches the pre-refactored response (tested web only, need to capture a mobile request).

Example web request: /api/v2/boost/feed?rating=2&rotator=1&sync=1&limit=150&as_activities=0&from_timestamp

The functionality appears limited, oddities found so far... (Checkbox Indicates Resolved)

• limit in request is not honoured

• rotator=1 is requested but not used anywhere

• sync=1 is requested but not used anywhere

• Iterator makes a request for X boosts, if Y of those boosts are invalid or can not be resolved the iterator will return X - Y boosts, requiring additional requests to cycle the iterator (which could return few or no results again)

• It is not implicit that boosts are always returned in ascending timestamp order

• It is not clear what a boost 'rating' is

• We ignore the offset requested from the iterator and instead override with the timestamp of the last boost in list with no explanation why we don't trust the iterator

added scoped labels

assigned to @gthouret

changed title from Investigate Oddities to Investigate Oddities

changed the description

/api/v2/boost/fetch seems to be a copy and paste of the original api/v2/boost/feed code (or could be the other way around). Unsure what calls this but I now know there are two APIs to maintain.

(Checkbox Indicates Resolved)

sync=1 is used by the /fetch API which in sets a sync flag which sets a hydrate flag in the iterator.

• Why different terms, why not make the naming consistent throughout?

hydrate set in the iterator makes the iterator fetch the entity and compare it's impressions value with the boost impressions value to determine if the boost has finished.

• It's called an Iterator, I am not expecting an iterator to contain this functionality, how could I have known this functionality is hidden away here, not to mention this is pretty inefficient within an API call.

In the hydrate functionality if we determine a boost has finished we call Expire::expire() which changes the boost state to complete and generates event and notification.

• Seems overkill for this one method to have it's own class, feels like it should be a part of Manager functionality

The increment option is used within hydrate to call Metrics::incrementViews() that increments some values in counters db table.

• Metrics references a Mongo database which AFAIK as been abandoned/decommissioned

• Couple of private methods that are unused anywhere (getBoostEntity, patchThumbs)

• Iterator $list member is public. It's only accessed by some dead code in the fetch API.

added scoped label

changed the description

BoostProvider

• What's the point in registering almost every boost class in DI? Most of these are not providing a service.

• In Iterator hydrate and increment flag functionality are mutually exclusive where as the code has them nested that implies some sort of dependency. i.e. We can't increment without hydrating which is not true.

If we make a call to expire the boost it's at this point that we bloat the boost object with the hydrated owner and user objects - which will likely never be used.

Should the Manager / Repository unset those objects before syncing?

In this case the owner and entity objects are many times the size of the dehydrated boost object.

Frontend has references to /api/v1/boost/fetch. So there's actually another set of duplicate functionality.

Rule of 3 threshold met.

Boost API Usage

Boost API	Platform	File Reference	Comment
/api/v1/boost/fetch	Web	src/app/lib/minds-sync/services/BoostedContentSync.js	BoostedContentSync.sync() function, uses sync=1 parameter. Appears to be syncing impressions from local storage to the backend
/api/v1/boost/fetch	Web	src/app/modules/ads/boost.ts	BoostAds.fetch() function, fetching for sidebar? Uses boost:offset:sidebar localstorage value for tracking offset
/api/v2/boost/feed	Web	src/app/common/components/featured-content/featured-content.service.ts	FeaturedContentService.fetch() function, relies on FeedService.fetch() which is where all the extra query params to boost feed are coming from (as_activities, from_timestamp, limit=150 Hard coded limit here.
/api/v2/boost/feed	Web	src/app/modules/newsfeed/boost-rotator/boost-rotator.component.ts	NewsfeedBoostRotatorComponent.load() function, relies on FeedService.fetch() again with limit=12 and offset=0 Should FeedService throw error if limit and offset not set i.e. not be setting it's own defaults
/api/v2/boost/feed	Web	src/app/modules/newsfeed/feeds/boost.component.ts	NewsfeedBoostComponent.load() function, relies on FeedService.fetch() again with boostfeed=true, limit=6 and offset=0 Should FeedService throw error if limit and offset not set i.e. not be setting it's own defaults
api/v1/boost/fetch/newsfeed	Mobile	src/newsfeed/NewsfeedService.js	NewsfeedService.getBoosts() function, queries with `limit
api/v2/boost/feed	Mobile	src/common/services/boosted-content.service.js	BoostedContentService.load() function, queries with limit=12, offset=0 Seems to be a basic service for fetching boosts using FeedsService. Can't find any usage within mobile app?!? I think this is code copied over from the Web app

marked the task is requested but not used anywhere as completed

The feeling I have is there is a massive amount of overhead/restriction in maintaining all of the various API endpoints for boost feeds and the services within the apps that consume those feeds and insert the 'boosted' content into pages.

I want this to be a backend only feature consisting of a single boost service responsible for creation, serving and reporting. Boosted content should be inserted into the existing feed APIs by a call to the boost service.

This will require change to the way those feeds are rendered on the frontend so that they identify and render the boosted content correctly, this render code is existing already just needs moving around.

This will be resolved in this epic

• Boost\Network\Iterator class has no spec test coverage.

• Boost\Network\Manager::getList() fetches boosts from elasticsearch when $opts['useElastic'] is set, then immediately makes a request to cassandra repository for the same boosts. I don't get what the reason for this is.

• Cassandra repository doesn't store all of the values for a boost object. @reviewed, @revoked, @rejected, @completed timestamps are derived from the current state and not actually recorded. I.e. you are in a completed state, you are not able to know the other timestamps if you query cassandra so not sure why we even have it?!?

While I'm at it what's with all the @ prefixes? https://stackoverflow.com/questions/39153596/elasticsearch-meaning-of-symbol suggests this is a convention from early releases of elasticsearch and not a requirement. Unnecessary clutter?

Discuss with @markeharding - outcome useful to have @ annotations to indicate timestamp value

added scoped label and automatically removed label

mentioned in commit 172cd64e

mentioned in commit cdac7730

mentioned in commit 47bad203

mentioned in commit 1e21eb78

mentioned in commit a00826eb

Discussed with @markeharding. Everything should be using /v2/boost/feed

Live frontend is requesting /api/v1/boost/fetch/content?limit=8&offset=1571869035752&rating=1 for sidebar content.

Investigating boosts schema in cassandra:

• Timestamps are a mixture of seconds and milliseconds, time_created is ms, last_updated is s, @ prefixed timestamps are ms
• Fullowner object is stored for both the boost object, the entity object within the boost and the owner of that entity

• In Boost\Network\Manager we are iterating a response object, hydrating the boost object and testing if it's valid then don't do anything with it because the object within the operator is not a reference to the one inside the iterator.

At some point there was some code that unset the boost from the response but this has been commented out long ago.

mentioned in commit 18092fe1

mentioned in commit 29bde787

mentioned in commit 529dcc73

mentioned in commit 9ed5d984

mentioned in commit f03e2fb4

removed label

marked the task We ignore the offset requested from the iterator and instead override with the timestamp of the last boost in list with no explanation why we don't trust the iterator as completed

marked the task Iterator makes a request for boosts, if of those boosts are invalid or can not be resolved the iterator will return - boosts, requiring additional requests to cycle the iterator (which could return few or no results again) as completed

Iterator related issues have been moved to #1067

added scoped label

added scoped label

changed weight to 4

added scoped label and automatically removed label

@gthouret can we get an estimate on this task?

Splitting this down into individual tasks with estimates.

marked the task in request is not honoured as completed

marked the task It is not clear what a boost 'rating' is as completed

closed