Skip to content

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

Merged
baywet merged 5 commits intomicrosoftgraph:devfrom
bougar:bugfix/511
Oct 2, 2020
Merged

[issue-511] Fix serialization of additional data on Collections#512
baywet merged 5 commits intomicrosoftgraph:devfrom
bougar:bugfix/511

Conversation

@bougar
Copy link
Copy Markdown

@bougar bougar commented Oct 1, 2020

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

ghost commented Oct 1, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@baywet baywet self-requested a review October 1, 2020 11:28
@baywet baywet self-assigned this Oct 1, 2020
@baywet baywet added this to the 2.3.0 milestone Oct 1, 2020
@baywet baywet linked an issue Oct 1, 2020 that may be closed by this pull request
AdditionalData not serialized for collections #511
Closed
@baywet
Copy link
Copy Markdown
Member

baywet commented Oct 1, 2020

@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 added 2 commits October 1, 2020 15:28
@baywet
- improves unit test to test aspects and not just containing information
df892dd
@baywet
- refactoring to reduce code duplication
97c772b 
- fixes a bug where additional data would be added to the wrong level for lists
baywet
baywet previously approved these changes Oct 1, 2020
View reviewed changes
@baywet
Copy link
Copy Markdown
Member

baywet commented Oct 1, 2020

thanks @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

baywet commented Oct 1, 2020

BTW @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/

MIchaelMainer
MIchaelMainer approved these changes Oct 2, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@MIchaelMainer MIchaelMainer left a comment

Choose a reason for hiding this comment

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

Reviewed via Teams. LGTM

@baywet baywet merged commit 8c3cb95 into microsoftgraph:dev Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MIchaelMainer MIchaelMainer MIchaelMainer approved these changes

@baywet baywet baywet left review comments

@zengin zengin Awaiting requested review from zengin

@nikithauc nikithauc Awaiting requested review from nikithauc

Assignees

@baywet baywet

Labels

hacktoberfest-accepted

Projects

None yet

Milestone

2.3.0

Development

Successfully merging this pull request may close these issues.

AdditionalData not serialized for collections

3 participants

@bougar @baywet @MIchaelMainer