With T303985: Suggested edits: make task feed reusable, the business logic is separated from the UI logic in the ext.growthExperiments.Homepage.SuggestedEdits. Similar abstraction should be applied to SuggestedEdits.php.
Description
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | None | T311850 [Epic] FY 2022-23 Growth Maintenance Work | |||
Open | None | T308054 Suggested edits: separate presentation and business logic in SuggestedEdits class (server-side) | |||
Open | None | T308541 Create common serialization method for SuggestedEdits and ApiQueryGrowthTasks | |||
Open | None | T308545 Remove duplicate data from task data feed |
Event Timeline
I was thinking about this today in the context of some awkwardness in our current client-side/server-side abstractions for fetching tasks (T312686: [betalabs] Suggested edits - the counter discrepancies for a recent example of that). Maybe we could think about an abstraction like:
- NewcomerTasksStore on the server-side, that has a method for fetch() that accepts the TaskSetFilters as well as a unique ID sent by the client
- The NewcomerTasksStore is responsible for:
- invoking ElasticSearch
- keeping track of page IDs found already, and storing them in a cache entry for the unique ID sent by the client
- applying filtering
- handling the "fetch more tasks" logic (deduplicating existing page IDs, filtering) and informing the caller when there are no more results
It seems like that kind of abstraction would simplify our client-side code, and maybe also make SuggestedEdits.php and ApiQueryGrowthTasks.php a bit simpler.
If we used a standard MVC setup, then getTaskSet(), resetTaskCache() and getSiteViews() would go to the model, state checks like isActivated(), getState(), canRender() etc. would go to the controller (plus it would mediate calls from the view to the model) and everything else would go to the view, which is about 900 out of 1000 lines. The part of the model that could likely be reusable elsewhere is something like 20 lines. So I'm not sure this is worth the effort.
keeping track of page IDs found already, and storing them in a cache entry for the unique ID sent by the client
A server side cache for tracking client state between requests seems like an antipattern to me. It's counter to the HTTP (and especially REST) philosophy, makes debugging harder locally and a lot harder in production, introduces edge cases around session loss, makes it complicated to support multiple browser windows from the same user in parallel. For a significant performance impact (like the one from CacheDecorator) that might be a reasonable trade-off, just for simplifying client-side code it's not, IMO.
Although it would probably make writing tests for it easier.
Would we want to separate presentation and business logic in just one homepage module, though? It seems like something we'd want to build into the whole homepage module structure, then.