feat: delete old config docs only after successful new config import #765
Conversation
…leteConfigOnlyIfSuccessfullImport
EvanHahn
changed the title
missed this comments. Gonna add this checks first thing tomorrow and re-ask for review! |
…leteConfigOnlyIfSuccessfullImport
…leteConfigOnlyIfSuccessfullImport
test-e2e/config-import.js
Outdated
| await project.importConfig({ | ||
| configPath: defaultConfigPath, | ||
| }) | ||
| assert.equal( |
There was a problem hiding this comment.
This is a failing test that I'm not being able to solve. I basically:
- load default config and take note of the number of presets
- load another config
- load the default config again
- Expect the number of presets to be the same as whe the first default config was loaded
The actual number of presets isnPresetsDefaultConfig + nPresetsValidConfig.
If I run it in a loop, I get the same number every time (so presets are being deleted...)
There was a problem hiding this comment.
This was just me misunderstanding Promises. Before I did:
Promise.all([[presetsToDelete], [fieldsToDelete], [translationsToDelete]].flat())
(basically flatting the list of stuff to delete and passing it to Promise.all)
Now I do:
Promise.all([Promise.all(presetsToDelete), Promise.all(fieldsToDelete), Promise.all(translationsToDelete)])
I think I'm maybe not doing deletions of presets, fields, translations in parallel now (only within themselves, so preset deletion is in parallel, field deletion too, etc). I'm doubting this because I'm not awaiting the inner Promise.alls
@EvanHahn any thoughts?
There was a problem hiding this comment.
Out of curiosity, why did this file change?
There was a problem hiding this comment.
two updates to that file:
key->tagKey- added color
Since I'm now counting loaded presets on that config, I wanted to have it correctly loaded
test-e2e/config-import.js
Outdated
| const nFields = (await project.field.getMany()).length | ||
| const nTranslations = (await project.$translation.dataType.getMany()).length | ||
|
|
||
| // load wrong config |
There was a problem hiding this comment.
Nit: this isn't just a wrong config, it's a bogus path.
| // load wrong config | |
| // load a non-existent config |
src/mapeo-project.js
Outdated
| translations.forEach(async ({ docRefType, docRef, docId }) => { | ||
| if (docRefType === 'field' || docRefType === 'preset') { | ||
| let doc | ||
| try { | ||
| doc = await opts[docRefType].getByVersionId(docRef.versionId) | ||
| } catch (e) { | ||
| opts.logger.log(`referred ${docRef.versionId} is not found`) | ||
| } | ||
| }) | ||
| ) | ||
| if (doc) { | ||
| toDelete.push(docId) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Sorry for being unclear earlier. We need to await all of these.
| translations.forEach(async ({ docRefType, docRef, docId }) => { | |
| if (docRefType === 'field' || docRefType === 'preset') { | |
| let doc | |
| try { | |
| doc = await opts[docRefType].getByVersionId(docRef.versionId) | |
| } catch (e) { | |
| opts.logger.log(`referred ${docRef.versionId} is not found`) | |
| } | |
| }) | |
| ) | |
| if (doc) { | |
| toDelete.push(docId) | |
| } | |
| } | |
| }) | |
| await Promise.all( | |
| translations.map(async ({ docRefType, docRef, docId }) => { | |
| if (docRefType === 'field' || docRefType === 'preset') { | |
| let doc | |
| try { | |
| doc = await opts[docRefType].getByVersionId(docRef.versionId) | |
| } catch (e) { | |
| opts.logger.log(`referred ${docRef.versionId} is not found`) | |
| } | |
| if (doc) { | |
| toDelete.push(docId) | |
| } | |
| } | |
| }) | |
| ) |
Also, should we make toDelete a Set? It's possible for multiple translations to refer to the same field/preset, right?
There was a problem hiding this comment.
yep, right. Changed on the last commit
should close #761
The approach I took is basically set a bunch of promises that only get resolved after a successfull
configImport. I think this approach is right