🎨 refactor the importer #8473

kirrg001 · 2017-05-21T13:36:44Z

refs #5422

🎊 ✌🏻

goals

we have two importer implementations, get rid of the old importer and continue with the new importer logic
remove unneeded logic to import specific cases (the specific cases are still supported, just a little bit smarter)

notes

most of the specific import cases are handled via the model layer
we have many importer tests, the main goal here was to keep them as is and they should pass with the new logic
there is a clearer differentiation between real error and problems/warnings (which we can show in the UI)
the logic of importing apps was ignored, it never actually imported apps
see more detailed descriptions in the commits

todo's

go over data importers and check if something is missing e.g. apps, clients, permissions!
test some exports of 1.0
import a large (~100mb) database
make a sanity check: compare an import carefully (tags, attached tags, posts, users, roles, images)
wait for confirmation: Can't import my backup #8365 (comment)
update the commit messages again
can i import null dates? yes, but they are transformed
more edge case testing

refs TryGhost#5422 - the new editor added the support for none null titles (null -> `(untitled)`) - all import tests relied on post.title null -> fails - we can support null titles after this PR if we want

no issue - this implementation was wrong

refs TryGhost#5422 - it was only supported to add roles by id e.g. roles = [1] or by object e.g. roles = [{....}] - we use the name strategy also for tags e.g. tags = ['my-tag'] - we support this for roles as well, this makes it easier when importing related user roles (because usually roles already exists in the database and the related id's are wrong e.g. roles_users)

refs TryGhost#5422 - if the json file contains null values for created_at/updated_at - can be handled on model layer and not on the importer layer

refs TryGhost#5422 - post or tag slugs are always safe strings - if e.g. you import a null slug, we can handle this, no need to crash or to cover this on import layer

refs TryGhost#5422 - basically nothing has changed here, just the none usage of the old importer and the result of problems/warnings while importing

refs TryGhost#5422 - this is the new integration of the importer - i have used a class inheritance mechanismn to achieve an easier readability and maintenace - most of the logic bits were copied from the old importer or modified/easified - here a list: - schema validation (happens on model layer) was ignored - allow to import unknown user id's (see TryGhost#8365) - most of the duplication handling happens on model layer (we can use the power of unique fields and errors from the database) - this PR has not concentrated on clear error messages or logs (this comes tomorrow) - i've added debug messages - the import is splitted into three steps: - beforeImport --> prepares the data to import, sorts out relations (roles, tags), detects fields (for LTS) - doImport --> does the actual import - afterImport --> updates the data after successful import e.g. update all user reference fields e.g. published_by (compares the imported data with the current state of the database) --> the background why we are doing it after the import is that we don't have to write complicated logic to detect outdated user references before we know which user get's which id --> plus: we don't care about the order of inserting the data --> keep in mind: we never ever import id's, that's why user references are usually outdated --> e.g. you import a post with `published_by:2`, but user 2 get's a new ObjectId 'X', we have to update the reference

refs TryGhost#5422

refs TryGhost#5422 - simply move to the correct folder

refs TryGhost#5422 - the markdown field will be removed soon, see TryGhost#8479 - if you import images the importer crashes if markdown is null e.g. the welcome post has no markdown field anymore (results in null)

kirrg001 · 2017-05-22T11:34:04Z

@cobbspur

This is ready for review.

I hope you find some issues with my PR, hard to cover every case alone.
I have imported lot's of exports, everything worked well for me.

Everything should work like before (+ even more cases are supported).

Don't be confused about the missing menu when importing a database, see #8307

- will show the user why entries weren't imported - e.g. Entry was ignored. We found a duplicate.

- return entry context objects as pretty json - do not return warnings for role duplications, no helpful information

- we show the affected json entries as one line in the UI

- if you have a post with duplicated tags, we tell you, so that you know

cobbspur

Tested and LGTM

This was referenced May 21, 2017

Duplicate entry found. Multiple values of '' found for . #8349

Closed

Can't import my backup #8365

Closed

kirrg001 force-pushed the 1.0.0-dev/fix-the-importer-logic branch 2 times, most recently from fca228e to 12a3056 Compare May 22, 2017 11:21

kirrg001 added 6 commits May 22, 2017 13:22

🎨 no default title if importing

9373481

refs TryGhost#5422 - the new editor added the support for none null titles (null -> `(untitled)`) - all import tests relied on post.title null -> fails - we can support null titles after this PR if we want

🐛 user model: fix getAuthorRole

68c1503

no issue - this implementation was wrong

🎨 base model: if created_at or updated_at are null

d491852

refs TryGhost#5422 - if the json file contains null values for created_at/updated_at - can be handled on model layer and not on the importer layer

🎨 support safeString with null

cd375e5

refs TryGhost#5422 - post or tag slugs are always safe strings - if e.g. you import a null slug, we can handle this, no need to crash or to cover this on import layer

🎨 update tests

b35e0b7

refs TryGhost#5422 - basically nothing has changed here, just the none usage of the old importer and the result of problems/warnings while importing

kirrg001 force-pushed the 1.0.0-dev/fix-the-importer-logic branch from 12a3056 to f9c2691 Compare May 22, 2017 11:24

kirrg001 added 4 commits May 22, 2017 13:25

🔥 remove old import folder

639ab4e

refs TryGhost#5422

🎨 move test files

685db7f

refs TryGhost#5422 - simply move to the correct folder

🐛 import images: markdown can be null

81be4e3

refs TryGhost#5422 - the markdown field will be removed soon, see TryGhost#8479 - if you import images the importer crashes if markdown is null e.g. the welcome post has no markdown field anymore (results in null)

kirrg001 force-pushed the 1.0.0-dev/fix-the-importer-logic branch from f9c2691 to 81be4e3 Compare May 22, 2017 11:25

kirrg001 changed the title ~~[WIP] 🎨 refactor the importer~~ 🎨 refactor the importer May 22, 2017

kirrg001 assigned kevinansfield and cobbspur May 22, 2017

kirrg001 mentioned this pull request May 22, 2017

🐛 care about post tags order when importing #8481

Merged

🐛 show error message when json handler can't parse file

0447ca1

kirrg001 mentioned this pull request May 22, 2017

Importer: Improve Post and Tag de-duplication #4685

Closed

kirrg001 unassigned kevinansfield May 22, 2017

🐛 do not request gravatar if email is null

493e13b

kirrg001 force-pushed the 1.0.0-dev/fix-the-importer-logic branch from ef84651 to 95df9ab Compare May 23, 2017 05:03

kirrg001 added 4 commits May 23, 2017 07:33

🙃 return problems/warnings after successful import

f4607be

- will show the user why entries weren't imported - e.g. Entry was ignored. We found a duplicate.

🎨 optimise warnings in importer

a4180c9

- return entry context objects as pretty json - do not return warnings for role duplications, no helpful information

🎨 error handler: return context information of error

6c4374e

🎨 optimise warning for user reference

377d8d8

Update tests again

367cddf

kirrg001 force-pushed the 1.0.0-dev/fix-the-importer-logic branch from 95df9ab to 367cddf Compare May 23, 2017 05:33

🔥 remove prettify context objects again

730891b

- we show the affected json entries as one line in the UI

kirrg001 assigned aileen May 23, 2017

kirrg001 added 3 commits May 23, 2017 13:31

✨ show warning for: detected duplicated tag

348f708

- if you have a post with duplicated tags, we tell you, so that you know

🐛 schema validation: fix valueMustBeBoolean translation

a12c254

🔥 remove context property from json parse error

957099a

kirrg001 force-pushed the 1.0.0-dev/fix-the-importer-logic branch from 812bd48 to 957099a Compare May 23, 2017 11:31

cobbspur approved these changes May 23, 2017

View reviewed changes

cobbspur merged commit 1f37ff6 into TryGhost:master May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎨 refactor the importer #8473

🎨 refactor the importer #8473

kirrg001 commented May 21, 2017 •

edited

Loading

kirrg001 commented May 22, 2017 •

edited

Loading

cobbspur left a comment

🎨 refactor the importer #8473

🎨 refactor the importer #8473

Conversation

kirrg001 commented May 21, 2017 • edited Loading

goals

notes

todo's

kirrg001 commented May 22, 2017 • edited Loading

cobbspur left a comment

Choose a reason for hiding this comment

kirrg001 commented May 21, 2017 •

edited

Loading

kirrg001 commented May 22, 2017 •

edited

Loading