Command to migrate legacy campaigns from Ashes -> Rogue #783

chloealee · 2018-11-21T19:27:19Z

What's this PR do?

Updates to campaigns table:
- adds secondary_causes column
- adds campaign_run_Id column
- adds internal_doc column
- makes fields nullable
Creates script to import legacy campaigns from Ashes to Rogue based on rules outlined here.
Creates test to make sure script is working as expected!

How should this be reviewed?

Did I capture all of the logic correctly?
Is there anything I am missing?
Feel free to ping me if you'd like to see a screen shot of the database and CSV to make sure all logic checks out! Another easy way to make sure this is correct is by looking at the sample CSV that the test imports and the logic there.
Finally, happy to share the SQL query @DFurnes and I came up to for the CSV export to also make sure that is correct since that is what this logic is based on!

Relevant tickets

Fixes task 3 in this Pivotal Card.

How to deploy

https://github.com/DoSomething/rogue/blob/master/docs/development/deployments.md

chloealee · 2018-11-21T19:30:06Z

One note - there is only one campaign that I found in the export that has NULL as the start_date. Since we don't want start_date to be nullable in Rogue and this is only one campaign, when we do the export from Ashes, can we just manually put its created_at date for the start_date?

katiecrane · 2018-11-27T16:50:18Z

app/Console/Commands/ImportAshesCampaigns.php

+            // Create the campaign if there isn't one already
+            if (! $existing_campaign) {
+                // If there is no campaign_id, set the campaign_run_id to the id.
+                if (is_null($legacy_campaign['campaign_id'])) {


Does this mean that in the csv runs will not have the campaign_id column populated?

@katiecrane great q! It will have the campaign_id column! However, after running the query, some campaigns didn't have this populated so wanted to account for these edge cases. Happy to share the CSV I'm working with too if that is helpful!

OH or does it mean that only CURRENT runs will have the `campaign_id populated?

@chloealee hmmmm does that mean that there are runs not attached to any campaign? I think seeing the CSV would help!

@chloealee ah okay I see, the first 7 rows are the ones with null. Are these actual campaigns? Seems like one of them is the DS app and 5 of them are in Spanish?

Yeah, these look like placeholder campaigns. I think it's safe to add them with the script, and we can delete later.

katiecrane · 2018-11-27T16:52:17Z

app/Console/Commands/ImportAshesCampaigns.php

+                if (is_null($legacy_campaign['campaign_id'])) {
+                    $campaign = $this->createCampaign($legacy_campaign['run_id'], $legacy_campaign);
+                    // If the campaign_id does not equal the next record's campaign_id or the previous record's campaign id, this is either the latest run or there is only one campaign run, so keep the original campaign_id as the id in Rogue.
+                } else if ($legacy_campaigns_csv->fetchOne($iterator) && $legacy_campaign['campaign_id'] != $legacy_campaigns_csv->fetchOne($iterator)['campaign_id'] && $legacy_campaign['campaign_id'] != $legacy_campaigns_csv->fetchOne($iterator - 1)) {


This might be easier to look at if this logic was extracted into a shouldUseCampaignId function? But I guess it might still all end up on one line. What do you think @chloealee ?

Yeah, this line is pretty intimidating. I'm not super concerned since this is a one-time script and we can easily check our work, but could be nice to clean up for readability.

katiecrane · 2018-11-27T17:00:28Z

tests/Console/ImportLegacyCampaignsCommandTest.php

+    public function testImportingLegacyCampaigns()
+    {
+        // Run the legacy campaign import command
+        $this->artisan('rogue:ashescampaignsimport', ['path' => 'tests/Console/example-legacy-campaigns.csv']);


yesssss thank you for making tests for this!! 🙌

+1! So helpful!

DFurnes

A few little thoughts, but I'm good to merge this as long as it's thoroughly tested!

DFurnes · 2018-11-28T15:14:41Z

app/Console/Commands/ImportAshesCampaigns.php

+    {
+        // Make a local copy of the CSV
+        $path = $this->argument('path');
+        info('rogue:legacycampaignimport: Loading in csv from ' . $path);


Can we just print these to standard output?

@DFurnes not sure what you mean by that?

@chloealee that would be doing $this->line('rogue:legacycampaignimport: Loading in csv from ' . $path). It'll print out in your terminal locally and when running on a one-off dyno in Heroku, it will print to the logs for that dyno (which is something we figured out semi-recently!)

oh!! this is so much better then taking up room in the Papertrail! updated!

I believe it will still log to PaperTrail

ooooo what are the benefits of logging this way?

@chloealee I was trying to remember that too! Looks like it's a memory thing and using info() a bunch of times can cause a memory leak

ah got it, thanks for explaining, @katiecrane !

DFurnes · 2018-11-28T15:20:13Z

app/Console/Commands/ImportAshesCampaigns.php

+            }
+
+            // See if the campaign exists
+            $existing_campaign = Campaign::where('id', $legacy_campaign['campaign_id'])->orWhere('id', $legacy_campaign['run_id'])->first();


Will this always find the "current run" that has the campaign ID? Say, if we create a campaign 6170 for campaign_id:6170,run_id:8, and then want to create a new campaign 7 for campaign_id:6170,run_id:7, I think this would just skip that second one, right?

Or does this work since we know campaigns are sorted in reverse order of old runs → current?

@DFurnes great q! Yes, this should work because the way we write the query and export the results to a CSV from Ashes, it'll always be reverse order of old runs -> current like you said.

Here's an example!

Alright, sounds good! 🤞

DFurnes · 2018-11-28T15:25:33Z

One note - there is only one campaign that I found in the export that has NULL as the start_date. Since we don't want start_date to be nullable in Rogue and this is only one campaign, when we do the export from Ashes, can we just manually put its created_at date for the start_date?

Yes, let's do that!

chloealee · 2018-11-28T18:37:03Z

app/Console/Commands/ImportAshesCampaigns.php

+                } else if ($legacy_campaigns_csv->fetchOne($iterator) &&
+                           $legacy_campaign['campaign_id'] != $legacy_campaigns_csv->fetchOne($iterator)['campaign_id'] &&
+                           $legacy_campaign['campaign_id'] != $legacy_campaigns_csv->fetchOne($iterator - 1)) {
+


@katiecrane @DFurnes I agree this is a little hard to read! Since it is just for this script, I'm not sure if it would make it that much easier to read if broken into a separate helper function but I played with the formatting to see if this would make it easier to read - lmk what you think!

@chloealee I love this! makes it much more readable!!!

ok great, thanks @katiecrane !

Yeah, much nicer! 👁

…g/editing/viewing a campaign

chloealee · 2018-11-30T16:24:58Z

app/Console/Commands/ImportAshesCampaigns.php

+                }
+
+                $this->line('rogue:legacycampaignimport: Created campaign ' . $campaign->internal_title);
+            }


@katiecrane @DFurnes I changed this log message to log the campaign title instead of the ID because I feel like that is more readable - lmk if you don't think this is a good idea though!

@chloealee whatever you think will be the most useful works for me 😄

DFurnes · 2018-11-30T19:29:25Z

app/Models/Campaign.php

@@ -21,7 +21,7 @@ class Campaign extends Model
     *
     * @var array
     */
-    protected $fillable = ['internal_title', 'cause', 'start_date', 'end_date'];
+    protected $fillable = ['id','internal_title', 'cause', 'secondary_causes', 'campaign_run_id', 'impact_doc', 'start_date', 'end_date', 'created_at', 'updated_at'];


Let's remove id, created_at, and updated_at from the fillable array & use Campaign::forceCreate in the script (since we don't want these to be mass-assignable elsewhere).

cool i didn't know about forceCreate ! Much better solution!

DFurnes · 2018-11-30T19:29:59Z

...00157_add_secondary_causes_run_id_impact_doc_and_make_causes_nullable_to_campaigns_table.php

+        Schema::table('campaigns', function (Blueprint $table) {
+            $table->string('secondary_causes')->index()->after('cause')->nullable()->comment('Secondary causes associated with legacy campaigns');
+            $table->integer('campaign_run_id')->index()->after('secondary_causes')->nullable()->comment('Campaign Run Id to reference when we run script to update signup and post ids after legacy campaign migration. This column can be deleted once migration and update script are complete.');
+            $table->string('impact_doc')->index()->after('campaign_run_id')->nullable()->comment('URL to proof of impact doc.');


We don't need an index on the impact_doc column since it won't be queried against.

DFurnes · 2018-11-30T19:31:41Z

app/Http/Controllers/Legacy/Web/CampaignsController.php

@@ -64,6 +64,7 @@ public function store(Request $request)
        $this->validate($request, [
            'internal_title' => 'required|string|unique:campaigns',
            'cause' => 'required|string',
+            'impact_doc' => 'required|string',


Let's use the url validator to enforce that this is a URL.

great call!

DFurnes · 2018-11-30T19:32:03Z

app/Http/Controllers/Legacy/Web/CampaignsController.php

@@ -126,6 +127,7 @@ public function update(Request $request, Campaign $campaign)
        $this->validate($request, [
            'internal_title' => 'string',


This is missing the unique:campaigns validation rule that we use when creating a new campaign.

You can include it here by specifying a campaign ID to ignore for the uniqueness check, e.g:

'internal_title' => [Rule::unique('campaigns')->ignore($campaign->id)],

very cool, thanks @DFurnes !

…rceCreate in script

DFurnes

This looks good to me!

chloealee added 10 commits November 16, 2018 14:59

creates import ashes campaigns command file

b784487

adds secondary_causes column in campaigns table

c23104e

WIP

c9c021b

adds campaign_run_id to migration

b308dc6

updates migration and gets first scenario working in import

a60f9e7

WIP comparing records

428914d

everything working except for sending to quasar and customer.io

cf84929

adds helper function and logic for last row in CSV

a51d35f

finishes script

f8c811d

adds test to test script

eec74e1

chloealee requested a review from a team as a code owner November 21, 2018 19:27

deletes TEST.csv

9297bc6

katiecrane reviewed Nov 27, 2018

View reviewed changes

DFurnes approved these changes Nov 28, 2018

View reviewed changes

spacing

42da6c7

chloealee commented Nov 28, 2018

View reviewed changes

chloealee mentioned this pull request Nov 29, 2018

Checklist: Migrate campaign metadata from Ashes & remove campaign run IDs. DoSomething/infrastructure#61

Closed

26 tasks

chloealee added 6 commits November 29, 2018 14:08

adds impact_doc to campaigns table

6301f23

updates front end and backend to add proof of impact doc when creatin…

42346f2

…g/editing/viewing a campaign

adds impact doc to script when creating campaign in rogue

dfd06e1

updates test csv to include impact_doc urls and campaign model factory

dac43cb

updates test

2df1ee9

updates to print log messages in standard output

419035c

chloealee commented Nov 30, 2018

View reviewed changes

chloealee added 2 commits November 30, 2018 12:19

deletes test.csv

0c7c40e

adds missing ;

b6a62b0

DFurnes reviewed Nov 30, 2018

View reviewed changes

chloealee added 4 commits November 30, 2018 15:51

removes id, created_at, and updated_at from fillble array and uses fo…

531a371

…rceCreate in script

removes index for impact_doc

3024dc1

changes impact_doc validation rule to url instead of string

6313732

adds unique:campaigns to internal_title in update

1e6e0de

chloealee force-pushed the import-ashes-campaigns-script branch from 7a82353 to 1e6e0de Compare November 30, 2018 21:26

DFurnes approved these changes Dec 3, 2018

View reviewed changes

chloealee changed the title ~~Command to migrate legacy campaigns from Ashes -> Rogue (pending with final OK from all teams on migration doc)~~ Command to migrate legacy campaigns from Ashes -> Rogue Dec 3, 2018

chloealee merged commit fb53be0 into master Dec 3, 2018

chloealee deleted the import-ashes-campaigns-script branch December 3, 2018 19:28

		@@ -126,6 +127,7 @@ public function update(Request $request, Campaign $campaign)
		$this->validate($request, [
		'internal_title' => 'string',

Command to migrate legacy campaigns from Ashes -> Rogue #783

Command to migrate legacy campaigns from Ashes -> Rogue #783

Conversation

chloealee commented Nov 21, 2018 • edited Loading

What's this PR do?

How should this be reviewed?

Relevant tickets

How to deploy

chloealee commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katiecrane Nov 27, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DFurnes left a comment • edited Loading

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

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

Choose a reason for hiding this comment

DFurnes 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

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

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

DFurnes left a comment

Choose a reason for hiding this comment

chloealee commented Nov 21, 2018 •

edited

Loading

katiecrane Nov 27, 2018 •

edited

Loading

DFurnes left a comment •

edited

Loading