search

DoSomething / gladiator

:guardsman: The DoSomething.org competitions platform.
2 stars 0 forks source link

Gladiator is returning a data object instead of an empty array for users not in a competition #405

Open itsjoekent opened 7 years ago

itsjoekent commented 7 years ago

I'm gonna lay out all of the steps to reproduce this issue because it's a little involved,

The bug is the last step, gladiator should return an empty array because the user never joined that competition.

here is an example url @weerd & myself used to debug this https://qa-gladiator.dosomething.org/api/v1/users?id=574ef3b0a59dbfa5768b456a&campaign_id=1283&campaign_run_id=1833

This was our workaround fix in Phoenix Next https://github.com/DoSomething/phoenix-next/pull/240#pullrequestreview-35646929

sbsmith86 commented 7 years ago

Assuming by competition you mean Contest here? We should always be returning user data and only adding a waitingroom (hasn't been split into a competition) or competition (was split into a competition already) property to the response if they user is in either.

When you ask for if a user joined the contest for campaign 50 and they haven't, you should be getting back something like: 

{
  "data": {
    "id": "5894e9437f43c217dd158b49",
    "first_name": null,
    "last_name": null,
    "email": null,
    "mobile": null,
    "signup": null,
    "reportback": null,
    "created_at": "2017-03-16T18:34:50+00:00",
    "updated_at": "2017-04-27T14:30:41+00:00",
    }
}

When they join the contest for campaign id 50, they get put into a waiting room and you would get back 

  "data": {
    "id": "5894e9437f43c217dd158b49",
    "first_name": null,
    "last_name": null,
    "email": null,
    "mobile": null,
    "signup": null,
    "reportback": null,
    "created_at": "2017-03-16T18:34:50+00:00",
    "updated_at": "2017-04-27T14:30:41+00:00",
    "waitingRoom": {
      "data": {
        "id": 33,
        "competition_start_date": {
          "date": "2016-04-25 00:00:00.000000",
          "timezone_type": 3,
          "timezone": "UTC"
        },
        "competition_end_date": {
          "date": "2016-05-30 00:00:00.000000",
          "timezone_type": 3,
          "timezone": "UTC"
        }
      }
    }
  }
}

When you ask if they joined a different contest that they haven't joined i.e campaign 70, again you should just get back the user data without an appending waiting room or competition.

If anything I am concerned as to why you are getting nothing back for the first request you made for campaign 50. Also, I can toooootally see a case for always appending waitingRoom or competition to the user and it just being empty if they are not in one, if that is easier. But I'm not sure I am understanding the bug 😬

weerd commented 7 years ago

Hmmm, yeah, maybe it should be intended behavior. So I think the discrepancy in return value is if the user is in the Gladiator system to begin with for any Contest.

So if we search for a user in a specific Contest and they've never been in any Contest previously, then it's an empty array.

But if we ask for a user in a specific Contest and they've been in any Contest prior but not in the one we specified, then it's an object w/ the user data.

If we ask for a user in a specific Contest and they're in it (either in WaitingRoom or Competition) then we get the full info.

Not sure what makes the most sense for expected results here, but up for discussing or going with whatever seems right.

weerd commented 7 years ago

Sorry. I keep hitting that damn "Close and comment" button 😬

sbsmith86 commented 7 years ago

I thought that is what was happening? so I think i'm still confused on where the bug is.

The bug is the last step, gladiator should return an empty array because the user never joined that competition.

I don't think this should be intended behavior. If they had previous activity in gladiator, then their user info is in gladiator, and i would expect a /users endpoint to return them.

Sorry if I explained that incorrectly but if the user has never been in gladiator, that would be the only time you get the empty array.

Definitely happy to discuss this endpoint more and flush out the functionality better.

itsjoekent commented 7 years ago

Sorry, I guess there was miscommunication between teams - I'm totally fine with it returning the user data, I think we were just operating under the assumption it wasn't supposed too

weerd commented 7 years ago

Just to clarify, I think the confusion is that, since we're filtering, we're specifically asking Gladiator (for this specified user, and campaign info) is there a user in the associated Contest? While there may be a user in the system, we're specifically asking if they're in a particular contest. If they are in the contest then return the user, otherwise tell me they're not in it. Makes it easier on the requesting service to then do boolean style checks... returned empty array, then nope, that user isn't in the contest and break from logic, otherwise returned data back, continue with rest of logic.

We can totally work with how it is for now, just maybe something to think about a potential update in the future if it makes sense.

sbsmith86 commented 7 years ago

@weerd I had sometime this sprint to think about this a little more and I sort of think it would be a good idea to split this functionality into two endpoints to answer different questions.

Thoughts? I would do this work under a v2 version of the API so it wouldn't break anything we already have out there, but I think this makes sense for a better API experiences going forward.

cc @DFurnes

DFurnes commented 7 years ago

I'm really into framing the request in terms of contests and then filtering based on campaign, run, and/or user ID. I do wonder if we could keep this more RESTful by fitting it all into a /contests resource, and then exposing additional information (for example, for data folks) using an ?include= parameter.

And 👍 to the suggested logic for /user.