Batch tutorial: create placements directly - #332
Conversation
| rank: species | ||
| status: 20 # ICNP | ||
|
|
||
| nanobdellaceae: |
There was a problem hiding this comment.
I feel some tension between using fixtures with names that describe the fixture (e.g. valid_name or unpublished_name) and then just using the name like here. Not sure what direction to go in.
There was a problem hiding this comment.
I think mixing them up is ok, depending on the aim of the test(s) using the fixture
| name_attributes = i.except('parent', 'incertae_sedis') | ||
| placement_attributes = { | ||
| parent: i['parent'], incertae_sedis: i['incertae_sedis'], | ||
| incertae_sedis_text: i['description'], preferred: true |
There was a problem hiding this comment.
Should we only carry over description if incertae_sedis is true? I think so.
There was a problem hiding this comment.
That's right, the guard you added later is indeed needed
| } | ||
|
|
||
| Name.new(name_attributes).tap do |name| | ||
| name.placements.build(placement_attributes.merge(name: name)) |
There was a problem hiding this comment.
Is it necessary to merge in name when using name.placements.build?
There was a problem hiding this comment.
This is gone, right? I guess it's inherited directly from i
| def save_batch_placement(name, attributes) | ||
| # Reuse matching alternatives and demote the current placement before | ||
| # promotion so the name never attempts to save two preferred placements. | ||
| placement = name.placements.find_or_initialize_by( |
There was a problem hiding this comment.
This is now the 3rd place in the code where we are doing something to set preferred placement. I have an idea for an alternative:
Add Name#preferred_placement_id and Name.belongs_to :preferred_placement, …. That way we enforce that there can only be one preferred placement. Thoughts, @lmrodriguezr ?
There was a problem hiding this comment.
I'll try and see what this looks like in another branch.
There was a problem hiding this comment.
Yes, that's certainly a better pattern. Migration might be tricky, but we should be able to do it with a simple rake task if it gets too complex for a change block.
09057f0 to
2304b7b
Compare
2304b7b to
f967e96
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
f967e96 to
7281549
Compare
| def ensure_consistent_placement | ||
| if parent_id.present? || incertae_sedis.present? | ||
| placement_incertae_sedis = | ||
| if saved_change_to_incertae_sedis? |
7281549 to
145ea82
Compare
|
@lmrodriguezr I think this is finally ready for review. |
This is somewhat unrelated to the pull request, but it allowed me to do some local testing
lmrodriguezr
left a comment
lmrodriguezr
left a comment
There was a problem hiding this comment.
This was long overdue in my to-do list, thank you!!
3 participants
I think the batch tutorial predates the
Placementclass. Currently it creates placements implicitly by settingName#parentandName#incertae_sedis, which then creates placements viaName#ensure_consistent_placementwhich is invoked as anafter_savehook.In preparation for #173 I have refactored the batch tutorial to create placements directly and added a bunch of tests verifying current behavior.