refactor: Move CSV imports into queued jobs #3407

rjmackay · 2018-11-20T02:21:07Z

This pull request makes the following changes:

Move csv/:id/import handler into CSVController
Replace generic ImportUsecase with ImportCSVUsecase since it was already specific
Move code from ImportListener (and a little from the controller) into ImportCSVPostsUsecase
Trigger ImportPosts event and then ImportPostsJob from ImportCSVUsecase
Add ImportPostsJob to run ImportCSVPostsUsecase

Cannot land till after #3371

Test checklist:

composer test
I certify that I ran my checklist

Ping @ushahidi/platform

rjmackay · 2018-11-20T02:22:37Z

app/Http/Controllers/API/CSV/CSVController.php

@@ -18,4 +18,21 @@ protected function getResource()
    {
        return 'csv';
    }
+
+    public function import(Request $request)


Moved here since this is now attached to the csv resource.

rjmackay · 2018-11-20T02:22:50Z

app/Jobs/ImportPosts.php

+    {
+        $usecase->setIdentifiers(['id' => $this->csvId]);
+
+        $results = $usecase->interact();


Just fires off the usecase :)

rjmackay · 2018-11-20T02:23:43Z

src/Core/Usecase/CSV/ImportCSVPostsUsecase.php

+use Ushahidi\Core\Tool\FileReader;
+use Ushahidi\Core\Usecase\Concerns\VerifyEntityLoaded;
+
+class ImportCSVPostsUsecase implements Usecase


Most of this came from Import listener, and prior to that from the old ImportUsecase

rjmackay · 2018-11-20T02:23:59Z

src/Core/Usecase/CSV/ImportCSVPostsUsecase.php

+    // - Provides dispatch()
+    use DispatchesEvents;
+
+    public function __construct(


Made this much more concrete because it's actually easier than using the factories

rjmackay · 2018-11-20T02:24:42Z

src/Core/Usecase/CSV/ImportCSVPostsUsecase.php

+        $file->fwrite($contents);
+
+        // Get records
+        // @todo read up to a sensible offset and process the rest later


Follow up step would be to batch these, similar to the Export job... but doing the simple version first

rjmackay · 2018-11-20T02:25:05Z

src/Core/Usecase/CSV/ImportCSVUsecase.php

+        $this->repo->update($entity);
+
+        // ... dispatch an event and let other services know
+        $this->dispatch($entity->getResource(). '.import', [


All we do here now is update status and trigger the import event

coveralls · 2018-11-20T02:29:44Z

Coverage increased (+0.01%) to 20.531% when pulling 181cae5 on refactor/csv-import-in-job into d24496b on develop.

rjmackay · 2018-11-20T02:37:13Z

app/Jobs/ImportPostsJob.php

+    {
+        $usecase->setIdentifiers(['id' => $this->csvId]);
+
+        $results = $usecase->interact();


Just fires off the usecase :)

- Move csv/:id/import handler into CSVController - Replace generic ImportUsecase with ImportCSVUsecase since it was already specific - Move code from ImportListener (and a little from the controller) into ImportCSVPostsUsecase - Trigger ImportPosts event and then ImportPostsJob from ImportCSVUsecase - Add ImportPostsJob to run ImportCSVPostsUsecase

- Remove Import command - Remove MappingTransformer - Remove ddeboer/import and symfony/property-access

rowasc

I think this should be OK to merge (after testing it)

refactor: Remove `./artisan import` command and MappingTransfomer

…t-in-job Conflicts: app/Providers/AppServiceProvider.php composer.lock src/App/Subscriber.php

…t-in-job

rjmackay · 2018-11-28T07:19:34Z

I've deployed this and #3408 to sad charlie. I have a few fixes incoming but it appears to work. however because we're not batching the imports we'll have to set the queue timeout really long before we deploy this

Pass the user id in the ImportPostsJob and run the job in that user context

rjmackay · 2018-11-28T07:21:20Z

src/App/Listener/ImportPosts.php

+
+    public function handle($id, Entity $entity)
+    {
+        $userId = $this->session->getUser()->getId();


Since we're sending this to another process we need to tell it which user to use

rjmackay · 2018-11-28T07:22:15Z

src/Core/Usecase/CSV/ImportCSVPostsUsecase.php

+        // load the user from the job into the 'session'
+        // the CSV itself doesn't save the creator so we're passing this
+        // in from the job queue
+        $this->session->setUser($this->getRequiredIdentifier('user_id'));


... I'm sort of wondering if I should do this at the job level instead now. and possibly make sure I set the user back to null after.

rjmackay · 2018-12-13T22:22:17Z

src/App/Listener/ImportPosts.php

+        $userId = $this->session->getUser()->getId();
+
+        // If csv-queue feature is enabled
+        if (Features::isEnabled('csv-queue')) {


@willdoran I completely forgot I added this. Basically this means without the feature flag this should be functionally the same as it is now: the CSV import runs synchronously in the HTTP request.

…t-in-job

rjmackay · 2018-12-13T22:26:26Z

src/Core/Usecase/CSV/ImportCSVPostsUsecase.php

+
+            // ... persist the new entity
+            try {
+                $id = $this->postRepo->create($entity);


In future I think we could collect these all into a array and send it to createMany and it should just work. We need to do it in batches to avoid keeping 10k posts in memory, but it should still dramatically speed up if you insert 500-1000 posts at a time.

rowasc · 2019-12-09T19:39:12Z

Keeping listed elsewhere until we decide if this can be merged yet

rjmackay requested review from rowasc and kinstella November 20, 2018 02:21

rjmackay commented Nov 20, 2018

View reviewed changes

rjmackay force-pushed the refactor/csv-import-in-job branch from 8f2ea84 to 81b9f2e Compare November 20, 2018 02:37

rjmackay commented Nov 20, 2018

View reviewed changes

rjmackay force-pushed the refactor/csv-import-in-job branch from 81b9f2e to fd2af23 Compare November 20, 2018 02:40

rjmackay mentioned this pull request Nov 20, 2018

Refactor to laravel style listeners and remove league/event #3408

Closed

2 tasks

refactor: Remove ./artisan import command and MappingTransfomer

88dcc55

- Remove Import command - Remove MappingTransformer - Remove ddeboer/import and symfony/property-access

rowasc approved these changes Nov 21, 2018

View reviewed changes

rjmackay added 3 commits November 26, 2018 16:48

Merge pull request #3409 from ushahidi/remove-console-importer

ae4bc43

refactor: Remove `./artisan import` command and MappingTransfomer

Merge remote-tracking branch 'origin/develop' into refactor/csv-impor…

6c8007d

…t-in-job Conflicts: app/Providers/AppServiceProvider.php composer.lock src/App/Subscriber.php

Merge remote-tracking branch 'origin/develop' into refactor/csv-impor…

5b3aa90

…t-in-job

rjmackay added 3 commits November 28, 2018 20:20

fix: Set auto_detect_line_endings in ImportPostsJob

0faa4d6

fix: Set user context in ImportCSVPostsUsecase

55e22d2

Pass the user id in the ImportPostsJob and run the job in that user context

fix(ImportCSVPosts): Don't try to add post to set if creation failed

be842e9

rjmackay commented Nov 28, 2018

View reviewed changes

refactor: Put queued CSV imports behind a feature flag

1d5bb40

rjmackay force-pushed the refactor/csv-import-in-job branch from 196f57e to 1d5bb40 Compare December 5, 2018 20:35

rjmackay commented Dec 13, 2018

View reviewed changes

Merge remote-tracking branch 'origin/develop' into refactor/csv-impor…

181cae5

…t-in-job

rjmackay commented Dec 13, 2018

View reviewed changes

rowasc closed this Dec 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Move CSV imports into queued jobs #3407

refactor: Move CSV imports into queued jobs #3407

rjmackay commented Nov 20, 2018 •

edited

rjmackay Nov 20, 2018

rjmackay Nov 20, 2018

rjmackay Nov 20, 2018

rjmackay Nov 20, 2018

rjmackay Nov 20, 2018

rjmackay Nov 20, 2018

coveralls commented Nov 20, 2018 •

edited

rjmackay Nov 20, 2018

rowasc left a comment •

edited

rjmackay commented Nov 28, 2018

rjmackay Nov 28, 2018

rjmackay Nov 28, 2018

rjmackay Dec 13, 2018

rjmackay Dec 13, 2018

rowasc commented Dec 9, 2019

refactor: Move CSV imports into queued jobs #3407

refactor: Move CSV imports into queued jobs #3407

Conversation

rjmackay commented Nov 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 20, 2018 • edited

Choose a reason for hiding this comment

rowasc left a comment • edited

Choose a reason for hiding this comment

rjmackay commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowasc commented Dec 9, 2019

rjmackay commented Nov 20, 2018 •

edited

coveralls commented Nov 20, 2018 •

edited

rowasc left a comment •

edited