(CM-947) ActiveRecord Refactoring ADR#772
Conversation
graycodes
force-pushed
the
CM-947-activerecord-refactoring-adr
branch
3 times, most recently
from
ae93d79 to
be51028
Compare
edavey
left a comment
edavey
left a comment
There was a problem hiding this comment.
This is really clear, thanks.
If you feel it's helpful to leave this open for a while, whilst you work through the TimePeriod work, in order to make refinements / adjustments, then I think you should consider doing that, rather than feeling like you need to finalise all the details immediately.
|
|
||
| #### 6. Two-Controller Pattern for Multi-Step Forms | ||
|
|
||
| Rather than use general-purpose multi-step "wizard" code (`Workflow`), it may be preferable, especially in the case of simple blocks (e.g. `TimePeriod`) to use separate controllers for different form steps: |
There was a problem hiding this comment.
We might perhaps be clearer here that this is about making the flow control explicit rather than derived from some other more abstract mechanism?
e.g would something maybe:
... to use separate
TimePeriodcontrollers to orchestrate the flow between the forms used to build the complete block
| Where possible, this experiment will aim to reuse existing behaviour which is packaged up in modules, e.g.: | ||
|
|
||
| - `Block::Edition` includes `::Edition::HasLeadOrganisation` for organisation validation | ||
| - `Block::TimePeriodDateRange` includes `DateValidation` for handling invalid multiparameter dates |
There was a problem hiding this comment.
I think maybe DateValidation isn't a good example of concerns we expect to reuse? As you say lower down, it actually belongs in the category of "behaviours" which we're confident we'll be able to implement in a simpler way?
There was a problem hiding this comment.
🤔 I had just copied this file exactly from your experiment branch so I was just going to commit it as-is so that we can reference it. Do you think I should update it?
There was a problem hiding this comment.
yes I think we can tweak to improve!
| 4. **Adding Fields:** Is it simpler to add a migration for a new column vs. updating a JSON schema? | ||
| 5. **Performance:** Do typed columns with proper indexes perform better than querying within JSON? | ||
| 6. **Testing:** Are traditional model tests more straightforward than testing schema validation? | ||
|
|
There was a problem hiding this comment.
Within this "Context" section, would it be helpful to have a visual aid? In my #553 PR narrative I included a couple of diagrams which attempted to illustrate the difference between:
- current state: complexity concentrated into few entities
- target state: complexity spread over numerous entities
and
There was a problem hiding this comment.
I have just added some simple diagrams to hopefully describe these changes in two separate ways. Let me know if you think there's more I could add to make things clearer.
A lot of work was put into the previous experiment to determine the approach to this refactoring. We would not want this work to be lost so I am committing this experimental ADR here so that we can reference it in future work. No changes have been made to this file. Original PR: #553
graycodes
force-pushed
the
CM-947-activerecord-refactoring-adr
branch
from
1b6744c to
73b4f2c
Compare
Based off the existing experimental work done to determine if this was a viable and helpful change to our approach, this ADR aims to encapsulate what we discovered. Much of the content of this ADR references the original experiment ADR. The original ADR contains more detail in some areas where it didn't feel necessary to re-state here. Original experiment: #553
graycodes
force-pushed
the
CM-947-activerecord-refactoring-adr
branch
from
73b4f2c to
6bec80d
Compare
2 participants
ADR for move to storing blocks as ActiveRecord models.
Based off the existing experimental work done to determine if this was a viable and helpful change to our approach, this ADR aims to encapsulate what we discovered. Much of the content of this ADR references the original experiment ADR.
The original ADR contains more detail in some areas where it didn't feel necessary to re-state here.
Original experiment:
#553