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.
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
Feature/sckan-324 - journey computation #381
base: develop
Are you sure you want to change the base?
Feature/sckan-324 - journey computation #381
Changes from 4 commits
ddd7d9c
22ba03f
ea1297f
5b03c6d
ae6bcb0
01436c0
b8a1d9e
50978d1
51b3838
b934de3
99d1238
466d8b3
cc18966
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why did we change from:
origins = list(connectivity_statement.origins.all())
to
origins = list(Origin.objects.filter(origins_relations__id=connectivity_statement.id).distinct())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason is here in the code - https://github.com/MetaCell/sckan-composer/blob/feature/SCKAN-324/backend/composer/services/graph_service.py#L24
It says:
so when generating the path - we do have the origins, but not the origin.name - which is a property in AnatomicalEntity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afonsobspinto did you do the check with the API call?
This happens only when we do the migration - possibly something with -
apps.get_model('composer', 'ConnectivityStatement')
in the migration file (ref - 0067 auto migration file). Also if you notice this is only for the@properties
and not for the fields.So if you put a breakpoint to the above mentioned code, and then run the migration to db - this should cause the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, during migrations django uses historical models that don't include python level constructs like @Property.
I think the new instruction works, but I would remove the .distinct(), I don't think it's needed in our context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before removing it, I want to confirm again with you if you are okay to remove the
distinct()
here, the reason I had that is because unlike vias and destinations - which are FK, origins is a M2M.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ManyToManyField in Django automatically ensures that the relationship between two models is unique. In other words, we know that we have a unique pair of ConnectivityStatement and AnatomicalEntity. Here's a simple test:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that said, if we have the time I would apply the same suggestion I mentioned here:
#404 (comment)