Fixed import for tags without slugs belonging to a post#10917
Conversation
closes TryGhost#10785 - The behavior for tags will now be similar to posts' one described in the docs - "The only strictly required field when importing posts is the title. Ghost will automatically generate slugs and set every other field to the default or empty." - The breaking change was introduced with: TryGhost@68d8154#diff-e712df50c0dc7cf33746eeff0564003cR97 (assumed there's always slug in imported object which is not true) - Added originalIdMap to the importer base class to track id substitution so it can be used when dealing with relational resource updates - Removed explicit use of 'this.stripProperties(['id']);' in beforeImport of base class because we need to assign and remove the id property in the same place to track this change - Only callingi 'this.stripProperties(['id']);' in settings/trusted_domain imports as the method wont be called otherwise - Expanded regression tests with new supported import case
allouis
left a comment
allouis
left a comment
There was a problem hiding this comment.
Overall changes seem to make sense, just come questions/comments about edge case and explicitness in tests 👍
| if (objectInFile.slug) { | ||
| importedObject = _.find(this.requiredImportedData[tableName], {originalSlug: objectInFile.slug}); | ||
| } else { | ||
| importedObject = _.find(this.requiredImportedData[tableName], {originalId: objectInFile.id}); |
There was a problem hiding this comment.
Do we need to check for the existence of objectInFile.id?
I noticed above that the originalIdMap doesn't necessarily contain an entry for every id - just trying to work out if there's an edge we're missing here?
There was a problem hiding this comment.
The objectInFile.id is always there because of this bit. But you are right, making it else if (objectInFile.id) makes it proof to accidental === undefined comparisons in case id assigning code changes in the future 👍
There was a problem hiding this comment.
Aaaah I see! Yup that makes sense
One more thing, what is the reason to attempt to match on slug first? Could we just always match on the id and remove the conditional?
There was a problem hiding this comment.
Very good point! Was thinking of slug matching as a fallback, but after rereading the previous statement there's no point in keeping it as the originalId will always be accessible 💡
allouis
left a comment
allouis
left a comment
There was a problem hiding this comment.
You can probably remove a few if statements now if you want - but up to you - merge at will
|
Leaving the
🚢 |
|
Good call 🎉 |
2 participants
closes #10785
The change adds a fallback to use the original
idproperty of imported object when handling relations, this was ignored previously. The main change here was aroundbeforeImportmethod in the Importer base class, so thatidproperty substitution is trackable.