-
Notifications
You must be signed in to change notification settings - Fork 30
4407 anatomical part #4588
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
Open
kleintom
wants to merge
88
commits into
development
Choose a base branch
from
4407_anatomical_part
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
4407 anatomical part #4588
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`rails g scaffold AnatomicalPart name:text uri:text uri_label:text is_material:boolean`
Previously it showed only originOf options (or vice versa) in the select, even when you flipped the direction.
This would be setting a new precedent - no other OR types currently allow creation in this form. The issue is: e.g., there is no association between CO and AP. Instead we have an origin relationship betweeen the two. (We probably don't want to maintain two lists?) Quick forms can, by design, only be used to create new association objects. So technically we can't have an AP quick form slice since APs are (rails-)associated with no models. So this is a compromise we may or may not want to proceed with. Currently the alternative would be to create the new AP from a New AP task or from the data New AP task, then search for that new AP in the AP search form on the quick form for Origin Relationship on a CO.
…4407 Means we don't have to remember to add a new type in 3 extra places everytime we want to add a type to observables.
Now New BA should auto-update BA subject/object options whenever a new BA type is added in rails.
…4407 This might still need work: only the 'new' table can be reordered, but keeping separate 'old' and 'new' lists with 'list = old + new' kind of breaks the expectations of `useSlice`. Also the existing way of updating position is broken in some cases (probably in other components as well) due to the way acts_as_list works when you add an item that has the same position as an existing item.
taxonomic_origin_object is the root of the chain that must exist from every AP back to a taxonomic (via otu) object. In turn, every taxonomic_origin_object determines a tree of descendant APs and optional leaf endpoints of origin relations with AP origin.
Necessary for validation. Also means you can't create an AP as origin object in quick forms (at least not without also requiring taxonomic_origin_object to be specified, which I don't think we want to do).
Constant on `old.object_type + new.object_type`. Maybe too much??
It's a little painful to have to click through the origin relationship modal like 4 clicks to get to 'New AP'.
Nice! I thought this was going to be trickier.
If we were expecting large spreading graphs then the original was probably better. We are not expecting that.
Time-costly oversight (to debug) of AI-added code :(
Fix/add some other specs
Thanks to ai for pointing me to `OriginRelationshipDestroyContext < ActiveSupport::CurrentAttributes` (same way `Current` works) when I/we couldn't get `destroyed_by_association` to work in this context. I changed destroy on AP to be disallowed unless it's the *terminal* object of the chain. It seemed like deleting an AP with child and Extract or Sequence would be a bad thing. You can now delete APs from the graph using the navigator menu, and it will stay on the updated graph. (Otherwise you'd have to search for each object of the chain and delete one at a time.)
|
Also delete lib/queries/prepartion_type/filter.rb, not sure how that got in there.
Code by chatgpt.
This is making me feel like we should just auto-set is_material (in the UI and in the model) in the CO/OTU case...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remaining TODOs:
is_materialcleanup: shouldn't be able to edit it in descendants; validate CO can only be is_material, etcis_materialvalidation for containerized APs