Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve form action domain model #696

Merged
merged 4 commits into from
Feb 11, 2025
Merged

Improve form action domain model #696

merged 4 commits into from
Feb 11, 2025

Conversation

gbirke
Copy link
Member

@gbirke gbirke commented Feb 10, 2025

In the next step of the refactoring, we'll introduce domain objects for
the form actions and call them "Actions" as well, so I'll rename the
form actions with the suffix `Url` to signify that they are URL strings.

Ticket: https://phabricator.wikimedia.org/T383980
We need to modify the form actions dynamically during A/B tests and want
to encapsulate the URL generation logic inside an API.

This commit is backwards-compatible with the existing `FormActions`
interface, but deprecates it.

Since the impression counting is non-reactive but tries to be reactive,
we don't put it as a dependency of `FormAction`. When it becomes
reactive, we should change the constructor of `FormAction` and hand over
the impression counting class as a dependency.

Ticket: https://phabricator.wikimedia.org/T383980
Use `FormActionCollection` with domain objects instead in both composables (`useFormAction` and `useFormActionWithReceipt`)

Create `fakeFormActions` fixture with `FormActionCollection` and use it in all tests that previously
used the string-based `formActions` injection.

Ticket: https://phabricator.wikimedia.org/T383980
@gbirke gbirke force-pushed the improve-form-action branch from d4c664f to f951655 Compare February 10, 2025 12:16
@moiikana moiikana merged commit 2c18b85 into main Feb 11, 2025
1 check passed
@moiikana moiikana deleted the improve-form-action branch February 11, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants