Skip to content

[issue-511] Fix serialization of additional data on Collections - #512

Merged
Vincent Biret (baywet) merged 5 commits into
microsoftgraph:devfrom
bougar:bugfix/511
Oct 2, 2020
Merged

[issue-511] Fix serialization of additional data on Collections#512
Vincent Biret (baywet) merged 5 commits into
microsoftgraph:devfrom
bougar:bugfix/511

Conversation

@bougar

Copy link
Copy Markdown

This PR tries to solve #511 issue.
I just modified the getChildAdditionalData method in order to support the serialization of additional properties on List items.

@ghost

Deleted user (ghost) commented Oct 1, 2020

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@baywet Vincent Biret (baywet) self-assigned this Oct 1, 2020
@baywet Vincent Biret (baywet) added this to the 2.3.0 milestone Oct 1, 2020
@baywet Vincent Biret (baywet) linked an issue Oct 1, 2020 that may be closed by this pull request
@baywet

Copy link
Copy Markdown
Member

Carlos Bouzón García (@bougar) thanks for the PR, I'll review it shortly. In the meantime, can you make sure you sign the CLA please? (see previous message)

@baywet

Copy link
Copy Markdown
Member

thanks Carlos Bouzón García (@bougar) for the pull request. I made a few modifications:

  • the unit test you added was testing presence of data but not that the data was "at the right place" (we have to revamp our unit tests for that aspect as well...)
  • there was a bug were the serialized data was not being added at the right level
  • I did some refactoring at the method had a lot of duplicated code and was hard to read
    Great job on catching the issue and starting the work to fix it!

@baywet

Copy link
Copy Markdown
Member

BTW Carlos Bouzón García (@bougar) if you sign up here and send a few other pull requests within the month, they'll send you a cool t-shirt!
https://hacktoberfest.digitalocean.com/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed via Teams. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AdditionalData not serialized for collections

3 participants