Copy shape on update #190
Copy shape on update #190
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #190 +/- ##
============================================
+ Coverage 57.14% 57.73% +0.58%
- Complexity 790 800 +10
============================================
Files 144 144
Lines 7288 7368 +80
Branches 847 864 +17
============================================
+ Hits 4165 4254 +89
+ Misses 2780 2771 -9
Partials 343 343
Continue to review full report at Codecov.
|
| // The output should contain a new backend-generated shape_id. | ||
| PatternDTO updatedSharedPattern = mapper.readValue(sharedPatternOutput, PatternDTO.class); | ||
| LOG.info("Updated pattern output: {}", sharedPatternOutput); | ||
| assertThat(updatedSharedPattern.shape_id, not(equalTo(sharedShapeId))); |
There was a problem hiding this comment.
We need to get into the habit of also checking that the database looks good in these tests. Funky stuff could happen in the database that aren't reflected in the output from gtfs-lib. Please add another few lines of code that make sure that the shape_id of the pattern with the secondPatternId doesn't equal the previous shared shape id.
|
Thanks for the review, @evansiroky. I've updated the test to check that the updated shape_id value persists in the database. |
| assertThat(newShapeId, not(equalTo(sharedShapeId))); | ||
| // Ensure that pattern record in database reflects updated shape ID. | ||
| assertThatSqlQueryYieldsRowCount(String.format( | ||
| "select * from %s.%s where shape_id='%s'", |
There was a problem hiding this comment.
I think we're almost there, but this assertion would pass if somehow the pattern with patternId was somehow updated to have the value of newShapeId. This could be improved by adding a where clause for the pattern id so that a more exact expectation of data integrity can happen.
There was a problem hiding this comment.
Are you suggesting that this where clause be changed to where shape_id=$newShapeId AND pattern_id=$secondPatternId?
There was a problem hiding this comment.
Great to see very specific assertions happening.
|
🎉 This PR is included in version 4.2.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
When multiple patterns share a reference to the same shape (for example, patterns for BART routes that travel along the same rail corridor but have short and long runs), editing the shape for one of these patterns can cause undesired changes for the other patterns. Because we don't really have a good way to handle shared shapes in the editor, we need an update to a shared shape to generate a new shape_id and leave the previous shape untouched. This PR handles this logic and also changed the log level for some noisy SQL logs to debug because they may be contributing to some performance issues for large SQL queries (see #146).