-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix: import of v2 documents with maps #1659
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1659 +/- ##
==========================================
+ Coverage 85.69% 85.74% +0.04%
==========================================
Files 604 604
Lines 30740 30759 +19
Branches 7895 8430 +535
==========================================
+ Hits 26344 26374 +30
+ Misses 4241 4064 -177
- Partials 155 321 +166
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
codap-v3 Run #5426
Run Properties:
|
Project |
codap-v3
|
Branch Review |
main
|
Run status |
Passed #5426
|
Run duration | 05m 55s |
Commit |
dc907fffed: Merge pull request #1659 from concord-consortium/188620026-fix-v2-map-importer
|
Committer | Kirk Swenson |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
36
|
Skipped |
0
|
Passing |
220
|
View all changes introduced in this branch ↗︎ |
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.
Looks good to me.
Would it be worth adding a test of this? I see there is a v2-graph-importer.test.ts
which uses a special mammals-graph doc to test it. So perhaps something similar for the map importer would be worthwhile.
974ce92
to
6d86b2d
Compare
I added jest tests. |
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.
👍🏻LGTM
Nice job, especially of making sense of ICodapV2MapLegacyStorage
[PT-188620026]
[PT-188614511]
Fixes several issues discovered while importing a number of legacy v2 documents with maps.
Note that it doesn't appear that v2 meaningfully supports the legacy v2 Map format, i.e. most of the properties are ignored and the Map reverts to its default behavior. We follow the same approach for v3, whereby the legacy types are acknowledged but not actually migrated.
Also fixes a v3 bug/limitation which prevented the Map component from supporting points and boundaries from the same data set.