Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Selected Position for Roster/TeamRoster #69

Merged
merged 1 commit into from Jan 13, 2021

Conversation

brisberg
Copy link
Contributor

@brisberg brisberg commented Jan 9, 2021

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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

@whatadewitt whatadewitt merged commit c89e1d0 into whatadewitt:master Jan 13, 2021
@brisberg brisberg deleted the fix-selected-position branch January 14, 2021 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants