CV2-6041: Verify the relationship target before establishing a new one. #2201
Conversation
melsawy
requested review from
caiosba,
vasconsaurus and
jayjay-w
as code owners
There was a problem hiding this comment.
@melsawy maybe this is a dumb question: but why not have this logic after the if r.nil? in line 164? Why do we want to check for this in line 161 and then again in 164, but then do something else?
There was a problem hiding this comment.
@vasconsaurus the first check against source_id & target_id and also I need to check another case for target_id only to avoid DB validation (unique index for target_id)
I did this before L164 as I need to skip any operation in case there is a relation with same target_id
There was a problem hiding this comment.
Thank you for the explanation @melsawy!
app/models/relationship.rb
Outdated
| r = Relationship.where(target_id: target_id).where.not(source_id: source_id).last | ||
| r = Relationship.where(source_id: source_id, target_id: target_id).where('relationship_type = ?', relationship_type.to_yaml).last unless r.nil? |
There was a problem hiding this comment.
Sorry @melsawy , it's still not clear to me what's the idea here (and I think that we need a comment in this code to explain it, like we had in the other parts of this method).
There was a problem hiding this comment.
Done and added more comments
|
Code Climate has analyzed commit 0f1a5a2 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (100% is the threshold). This pull request will bring the total coverage in the repository to 100.0% (0.0% change). View more on Code Climate. |
…e. (#2201) * CV2-6041: check relationship target before creating new one * CV2-6041: apply PR comments
Description
Verify the relationship target before establishing a new one to prevent triggering errors due to database unique validation.
References: CV2-6041
How has this been tested?
Re-run automated tests.
Checklist