Remove ability to write or query campaign_run_id. #798
Conversation
|
@katiecrane @chloealee This is ready for review! We can safely deploy this to QA to test as well, since we've updated the database there. We can then have a little testing party to make sure there aren't any edge cases we're not thinking of for this once it hits production. |
| $rejectedPosts = factory(Post::class, 'rejected', 3)->create(); | ||
| $userId = $this->faker->unique()->northstar_id; | ||
| factory(Post::class, 2)->create(['northstar_id' => $userId]); | ||
| factory(Post::class, 'rejected', 1)->create(['northstar_id' => $userId]); |
There was a problem hiding this comment.
This test was flaky since we'd sometimes randomly generate the same owner and non-owner Northstar IDs. I've updated the test to generate two separate IDs and use them for each batch of posts so it consistently passes.
There was a problem hiding this comment.
ah yes thank you for fixing this!!
| campaign.campaign_runs.current, | ||
| signup.campaign_run_id, | ||
| )} | ||
| Campaign Run Start Date: {campaign.start_date} |
There was a problem hiding this comment.
do we want this to just rad "Campaign Start Date"?
There was a problem hiding this comment.
We sure do! Updated that in 5b408d3.
|
I LOVE THIS PR SO MUCH!!!!! |
| * | ||
| * GET /activity?filter[]= | ||
| * @return void | ||
| */ | ||
| public function testActivityIndexWithNorthstarIdAndCampaignRunIdFilters() | ||
| { | ||
| $signup = factory(Signup::class)->create(['northstar_id' => 17, 'campaign_run_id' => 143]); |
There was a problem hiding this comment.
I think we should change the title of this test to testActivityIndexWithNorthstarIdAndCampaignIdFilters
There was a problem hiding this comment.
Ah, of course! Updated in 4227bd7.
tests/Http/PostTest.php
Outdated
| $response = $this->withAccessToken($northstarId)->getJson('api/v3/posts'); | ||
| $response->assertStatus(200); | ||
| $response->assertJsonCount(2, 'data'); | ||
| // Owners should only see their own pending/accepted posts, and not |
There was a problem hiding this comment.
it might be more clear to say "Owners should be able to see their own posts of any status, but not pending or rejected posts from other users" since we are showing this user their own post which has been rejected
There was a problem hiding this comment.
That reads a lot better! Updated in 4227bd7. 👍
|
@DFurnes this is amazing!!!! |
What's this PR do?
This pull request removes the ability to create new posts or signups with
campaign_run_idvalues or to filter on existing values (so?filter[campaign_run_id], for example, is a no-op). We can make a follow-up pull request in a few sprints to remove thecampaign_run_idfield from the database and transformers once and for all.How should this be reviewed?
I started by removing assertions that depended on
campaign_run_id, then removed the ability to read or write that value from the application. Rather than add some short-lived tests to ensure we can't write this value anymore, let's just have a quick testing party after we deploy this.Any background context you want to provide?
The plan is to deploy this once we've run the script to remove
campaign_run_idvalues in production, so that even if Ashes, Phoenix, and Gambit aren't immediately updated to stop querying or writing this value, we can safely ignore it here.Relevant tickets
References DoSomething/infrastructure#61.
How to deploy
https://github.com/DoSomething/rogue/blob/master/docs/development/deployments.md