whatadewitt / yahoo-fantasy-sports-api

NodeJS wrapper for the Yahoo! Fantasy Sports API
https://yahoo-fantasy-node-docs.vercel.app/
MIT License
200 stars 54 forks source link

Fix: Selected Position for Roster/TeamRoster #69

Closed brisberg closed 3 years ago

brisberg commented 3 years ago

This fixes "selected_position" not being properly extracted from the response payloads for team.roster and roster.players.

It seems that at some point, Yahoo changed their format of the payload without updating the documentation. I have replaced the nock-testdata with the latest real data for the same test team and date.

It is hard to see in this CL, but if you look for "selected_position", it was changed from:

{
   "player": [
      [
         // Other fields
         "selected_position": [ "..." ],
         // Rest of fields
      ],
   ],
}

To this:

{
   "player": [
      [
         // Player fields
      ],
      { 
         "selected_position": [ "..." ],
      }
   ],
}

Though I have updated the test data, there are no tests which verify the actual extraction of the data from the payloads. All of the tests are testing the existence of the API and the structure of the URLs (which have not changed).

whatadewitt commented 3 years ago

Sorry, I'm just seeing this! I'm going to review and get it merged in tonight... can you give me an idea of what type of game/league you're querying here? The data isn't always consistent so I just want to see it myself...

... and yea, the tests need work 😳

Thanks for contributing!

whatadewitt commented 3 years ago

Had a chance to look at this... ah the joys of Yahoo! data :D -- also found a bug in my sandbox app that I'm going to have to fix, so thanks again :D

The only thing I'd question (and I realize I'm really reaching here) but I notice in the data that I'm looking at, anyway, there's an "is_flex" flag on the selected position object. I feel like that's probably worth having as data so long as we're getting the proper data in there.

brisberg commented 3 years ago

No Problem!

You are correct that is_flex is set on selected position. Since there is no existing field on our Player model, do you have a preferred location to put it?

Unless it is trivial I'd prefer to keep this PR focused on the selectedPosition.position. Maybe you can do a follow up to add the field after this is merged.

brisberg commented 3 years ago

The new test data I provided was querying the same league/game/team they were before. (I looked up the league/team keys from the old test data payloads). I just ran the query again.

whatadewitt commented 3 years ago

That works for me... I have a couple of other small changes so I'll do a fast follow with that work!