Page MenuHomePhabricator

Suggested edits: separate presentation and business logic in SuggestedEdits class (server-side)
Open, Needs TriagePublic

Description

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.

Event Timeline

kostajh subscribed.

Tentatively moving to upcoming work.

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.

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.

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.