Refactoring and simplification of spam-related services [RUN ALL RSPEC] [RUN AS-IF-FOSS]
What does this MR do?
Overview
- Part of an ongoing series of refactorings to simplify the backend and frontend implementations of spam protection and CAPTCHA.
- Follow on to MR !56190 (merged), see related discussion there
- Removes spam checking from issue create/update via boards, as that currently has existing UI bugs due to CAPTCHA not being supported properly. Whether or not boards should have spam/CAPTCHA coverage is a separate discussion, and just one of many for many areas of the app. See https://gitlab.com/gitlab-org/gitlab/-/issues/29400#note_598479184 for context.
- Changes references to
request.referrer
torequest.referer
, for consistency (See also !63947 (merged) for some renames in unrelated areas of the app).
Details of refactoring and simplification of spam-related services:
- Decouple spam-related services to remove dependency on request, use SpamParams parameter object instead
- Expand SpamParams fields to replace existing direct usages of request
- Pass SpamParams through services exclusively via constructor arg (required for creation, optional for updates)
- Hardcode SpamLog
via_api
to true, it should be deprecated and removed (everything is via API now) - Make necessary changes to issue/issueable services constructor calls to accommodate addition of spam_params named argument as last argument.
Note on service constructor signatures and named arguments:
- We added a
..., spam_params:
named argument as the last argument to several Services' constructors. - In order to avoid having to wrap the previously-last hash positional arg in braces and other strangness, we have instead converted all relevant service constructor signatures to named args. This was done in two previous separate large MRs:
This MR is big! Can it be broken up?
That is the nature of these MRs which are modifying the constructor signatures which involve multiple services and many calls to them. It touches many areas of code, but it's all cohesive and makes sense to do together. See !59182 (merged) for a more in-depth discussion of this topic.
The one part which COULD be pulled out is the removal spam checking from issue create/update via boards (see bullet above). However, that is only a few lines and would not contribute much to reducing the overall size of the MR. If there are strong reviewer/maintainer feelings that this should be pulled out to a separate MR, I can do it, but for now I want to get the review process started.
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have not included a changelog entry because there are no user facing changes.
-
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
See the testing section of the CAPTCHA epic for detailed instructions on how to test spam/CAPTCHA in a dev environment.
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done