Conversation
ayumi
left a comment
ayumi
left a comment
There was a problem hiding this comment.
code seems good, let's await an android test by @SergeyZhukovsky
test/client/recordUtil.js
Outdated
|
|
||
| t.test(`${t.name} resolves same data cross-platform on laptop and android`, (t) => { | ||
| t.plan(1) | ||
| const androidRecordsAndExistingObjects = require('./data/resolveAndroid').data |
There was a problem hiding this comment.
minor: I like to call test data "fixtures"
There was a problem hiding this comment.
it may help to add a comment explaining how the ./data/resolveAndroid and ./data/resolveLaptop fixtures had been generated, such that someone could go and regenerate them.
| module.exports.resolveRecords = (recordsAndExistingObjects) => { | ||
| let resolvedRecords = [] | ||
| recordsAndExistingObjects.forEach((item) => { | ||
| if (item) { |
There was a problem hiding this comment.
is the item truthiness check necessary? in mergeRecords() we do a similar forEach() while assuming the values of item[0] and item[1].
There was a problem hiding this comment.
maybe not necessary at the moment but better to be safe here since resolveRecords is an exported function
browser-laptop sends 'parentFolderObjectId: []' for top-level folders whereas android sends 'parentFolderObjectId: null'. for consistency, they should just be normalized to be the same value. fix #107
|
It looks good. I verified it with the current pull request and laptop syncs a folder structure properly now. Thanks! |
3 participants
browser-laptop sends 'parentFolderObjectId: []' for top-level folders whereas
android sends 'parentFolderObjectId: null'. for consistency, they should just
be normalized to be the same value.
fix #107
TODO check if this fixes #100
test plan:
yarn distto build and copy the sync library over to browser-laptop. in browser-laptop, editjs/constants/appConfigand changeisProductionto true. then sync to "whisker..." group in browser-laptop. the hierarchy should be the same on android and laptop.