search

citizensandtech / CivilServant

CivilServant supports communities on reddit to conduct your own A/B tests on the effects of moderation practices, and share those results to an open repository of moderation experiments.
http://civilservant.io
MIT License
22 stars 5 forks source link

moderator_controller.py needs to handle multi-page API results #52

Open dantaeyoung opened 1 month ago

dantaeyoung commented 1 month ago

archive_mod_action_page in moderator_controller.py needs to handle multi-page API results (see: comment_controller.py)

natematias commented 1 month ago

Hi Dan, thanks for posting this requirement!

To my knowledge, this method does handle multi-page API results, which is why it takes after_id as an argument.

If you look at app/controller.py::fetch_mod_action_history, you will see how paging is handled.

For reasons of making the controller actions callable by RQScheduler, app/controller.py is the listing of the actual jobs which call the methods in the controllers, which is why you might not have seen all of the logic of the controllers in the controller files. The test for app/controller.py will handle testing for all of the jobs.

natematias commented 1 month ago

If you still have questions, feel free to keep the issue open. If it answers your question, I leave it to you to decide whether the issue should be closed.

dantaeyoung commented 1 month ago

Thanks, Nathan! Ah, I see. Follow-up question, then:

We've been working off of test_messaging_experiment_controller.py, which doesn't use app/controller.py::fetch_last_thousand_comments in the test but re-implements the same code. https://github.com/citizensandtech/CivilServant/blob/7111128cce8575060da41cbf336fe7ac9e1cd01c/tests/test_messaging_experiment_controller.py#L97-L101

Similarly, app/test_controllers.py also doesn't use app/controller.py::fetch_last_thousand_comments and re-implements the same comment fetching code. https://github.com/citizensandtech/CivilServant/blob/7111128cce8575060da41cbf336fe7ac9e1cd01c/tests/test_controllers.py#L551-L555

Is there a reason to not use app/controller.py::fetch_last_thousand_comments in the tests themselves? Otherwise, it seems like the code logic is being replicated at least three times, unless I'm missing something..