Always update signup for latest (or empty) run, if it exists. #788
Conversation
This API call is never made here, so doesn't need to be mocked.
| @@ -90,9 +90,6 @@ public function testNotCreatingDuplicateSignups() | |||
| { | |||
| $signup = factory(Signup::class)->states('contentful')->create(); | |||
|
|
|||
| // Mock the Blink API call. | |||
| $this->mock(Blink::class)->shouldReceive('userSignup'); | |||
|
|
|||
| $response = $this->withAccessToken($signup->northstar_id)->postJson('api/v3/signups', [ | |||
There was a problem hiding this comment.
just curious why you deleted here!
There was a problem hiding this comment.
We're not creating a signup, and so this method is never called. Mocking it is unnecessary, and potentially harmful since if we started sending userSignup events here due to a bug we'd never know it.
There was a problem hiding this comment.
ohhh right because the signup already exists since we are creating it in the model factory - got it!
| { | ||
| $signup = Signup::where([ | ||
| 'northstar_id' => $northstarId, | ||
| 'campaign_id' => $campaignId, | ||
| 'campaign_run_id' => $campaignRunId, | ||
| ])->first(); | ||
| ])->orderByRaw('campaign_run_id IS NULL') |
There was a problem hiding this comment.
ah interesting! So orderByRaw just makes sure that the NULL values are at the bottom?
There was a problem hiding this comment.
Oh! It should be the exact opposite, that was a mistake. Updated in c7deaa4.
The idea is that if we make new signups where campaign_run_id is null, we want those to be prioritized over old ones where this is a number. So the final query looks like kinda like this:
SELECT * FROM signups
WHERE northstar_id = "5543dfd6469c64ec7d8b46b3" AND campaign_id = 1144
ORDER BY campaign_run_id IS NULL DESC, `campaign_run_id` DESC LIMIT 1There was a problem hiding this comment.
@chloealee I added a new test for this in 9d794d2. Thanks for catching!
|
Ah got it - thanks for explaining and putting in that extra test! I hadn't seen |
| @@ -65,7 +65,7 @@ public function store(Request $request) | |||
| $northstarId = getNorthstarId($request); | |||
|
|
|||
| // Check to see if the signup exists before creating one. | |||
| $signup = $this->signups->get($northstarId, $request['campaign_id'], $request['campaign_run_id']); | |||
| $signup = $this->signups->get($northstarId, $request['campaign_id']); | |||
There was a problem hiding this comment.
@DFurnes - I think this line may what's causing some unexpected behavior in Gambit, where we're not seeing new signups from yes responses to the TYF broadcast when a user has signed up for the campaign in a previous run.
There was a problem hiding this comment.
Oh, shoot. You’re totally right. We can revert this PR this morning to resolve that problem.
I think our best course of action is to avoid removing runs from applications until we’ve run the script in Rogue, since otherwise we don’t have a reliable way to know whether or not a signup is new. (Or we could adjust this to find the latest run for all signups on a given campaign ID, but I’m thinking that adds some unnecessary complexity and a chance we introduce other bugs.)
There was a problem hiding this comment.
I’ll update the order-of-operations issue to reflect this. cc: @chloealee @weerd
What's this PR do?
This pull request updates Rogue's
POST v2/signupsandPOST v3/signupsto always update the latest run if it exists for a campaign, rather than making a new one where thecampaign_run_idis null. This will allow us to stop sending Campaign Run IDs (and run the script in #787) without breaking this functionality for applications that rely on it.How should this be reviewed?
I added a failing test in 3e5590a which demonstrates the issue (we create a new signup when we shouldn't), and you can see the issue is fixed in e51a989. This doesn't break any other tests, and I don't believe we rely on the prior unexpected behavior anywhere.
Any background context you want to provide?
This was flagged by @weerd when testing Phoenix's new sans-runs signup logic.
Relevant tickets
Fixes #162426572.
How to deploy
https://github.com/DoSomething/rogue/blob/master/docs/development/deployments.md