WordPress / five-for-the-future

Plugins and themes for the Five for the Future subsite
https://wordpress.org/five-for-the-future/
46 stars 21 forks source link

Add activity handler to Profiles helper #191

Closed iandunn closed 2 years ago

iandunn commented 2 years ago
iandunn commented 2 years ago

@dd32 xref https://github.com/WordPress/wordpress.org/commit/cce31a702dd65436e8d4229aa02c86db06fa193f, did you see any reason why the handle_{wordcamp|meetup|polyglots}_association() functions need to remain, instead of using the new generic one? They seem functionally identical for the most part.

iandunn commented 2 years ago

refactored a bit in https://github.com/WordPress/wordpress.org/commit/5dd0b26389f805c2b09c7fc09f606e1acc09aa99 and https://github.com/WordPress/wordpress.org/commit/10c831913635b6dbc356e009b7832bb408538cdb.

for activity, i think it makes sense to just call api() directly, because you have to pass a bunch of bp_activity_add() args anyway. a wrapper would really only remove the 'action' => 'wporg_handle_activity' one

on the buddypress side, it might make sense to remove handle_learn_activity(), and just call a generic handler in most cases, since all that that function really does is safelist some parameters, which are already trusted b/c they were called by our custom code. that would just leave action, which could be defined by the caller.

i'll test that out w/ Learn, and then apply it to handle_slack_activity() and handle_wordcamp_activity() as well. the rest of the handlers would be good to refactor at some point, but i think that's good enough for now.

dd32 commented 2 years ago

dd32 xref WordPress/wordpress.org@cce31a7, did you see any reason why the handle_{wordcamp|meetup|polyglots}_association() functions need to remain, instead of using the new generic one? They seem functionally identical for the most part.

There's no reason for them to remain really..

I plan to remove the polyglots one by refactoring the generic handler & updating the generic handlers to accept an array of user_ids. Polyglots was blocked by https://buddypress.trac.wordpress.org/ticket/8688 at present, as it's been impossible to test without getting OOM issues :) I might commit these now that I know of a workaround for polyglots.

for activity, i think it makes sense to just call api() directly, because you have to pass a bunch of bp_activity_add() args anyway. a wrapper would really only remove the 'action' => 'wporg_handle_activity' one

I think a wrapper would be nice, just to remove some of the specific implementation, and make it more obvious what the method is doing. I think your API handler in https://github.com/WordPress/wordpress.org/commit/10c831913635b6dbc356e009b7832bb408538cdb#diff-8446b890264a2c54ce3c6913911f49bb7b2424c379e8d6766c29aa6387d92578R226-R236 could just have a helper variant of it that passes the data onwards through the API.

iandunn commented 2 years ago

I think a wrapper would be nice

🤷🏻‍♂️ , I didn't see the need for this ticket, but I'm not opposed to it.

refactor profile handlers to simplify and remove duplication. maybe remove them entirely in favor of a generic handler

I simplified the Learn handler as an example, so that new ones we add don't need all the extra boilerplate. I'm gonna consider that "good enough" for now since there's a lot more work to do in this milestone.