[FND-209] Build service & job to generate project specific variants for form configs - #24658
Conversation
d855722 to
20e7e57
Compare
dfriquet
left a comment
dfriquet
left a comment
There was a problem hiding this comment.
Easy to read 👍
One edge case to solve, and some spec SRP.
There was a problem hiding this comment.
This spec is overlapping a lot on the service one. Some assertions rely on service internals, like created data structures for example.
In my opinion, the job spec should be mostly spy-based, asserting service calls and stubbing the result. All that in order to check its responsibilities: handing the service the right arguments and counting outcomes to report.
There was a problem hiding this comment.
I see the job spec more as an integration of the entire feature.
There was a problem hiding this comment.
I don’t see what under-the-radar moving parts that integration exercises. I read already covered cases by the service’s specs, and no assertion on the @built/@unchanged/@failed trio for example, or the transaction-in-transaction outcome.
But this is a spec file for a one-time job, I can live with that 👌
| result = Projects::Types::SwitchVariantService | ||
| .new(user:, model: project) | ||
| .call(source: type, target: variant) |
There was a problem hiding this comment.
This will fail on every archived project: SwitchVariantService validates Projects::ManageTypesContract, ending up with unless project.active? || project.being_archived? in UserPermissibleService#allowed_in_single_project?.
As the BuildVariantFromProjectService and Projects::Types::SwitchVariantService are not in the same transaction, the failure of the latter will leave the creation of the former orphan.
I don’t know how we should handle this case. We cannot narrow the ProjectType query to handle only active (and being archived) projects, leaving archived project without customized form configuration, can we?
There was a problem hiding this comment.
Usually we pass the EmptyContract in cases like this. I think this should also be the correct pattern here. Adding a spec and putting everything in one transaction
|
One follow-up question: the effective removal of custom field configuration screen is expected in another PR? |
Yes, this is only the service logic |
…lback when switching fails
dfriquet
left a comment
dfriquet
left a comment
There was a problem hiding this comment.
TIL about the EmptyContract ➕ User.system 👍
There was a problem hiding this comment.
I don’t see what under-the-radar moving parts that integration exercises. I read already covered cases by the service’s specs, and no assertion on the @built/@unchanged/@failed trio for example, or the transaction-in-transaction outcome.
But this is a spec file for a one-time job, I can live with that 👌
2 participants
Ticket
https://community.openproject.org/projects/FND/work_packages/FND-209/activity
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Merge checklist