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.

Sign up for GitHub

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

Jump to bottom

PostReaction struct improvements #160

Merged
merged 15 commits into from
May 13, 2020
Merged

PostReaction struct improvements #160

merged 15 commits into from
May 13, 2020

Conversation

leobragaz
Copy link
Contributor

Description

This PR improves the PostReaction type structure to allow a better integration with middle layer applications.
Closes #157.

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Wrote integration tests (simulation & CLI).
  • Updated the documentation.
  • Added an entry to the CHANGELOG.md file.
  • Re-reviewed Files changed in the Github PR explorer.

@leobragaz
updated post reaction structure
e2a4fa6 
fixed tests:
TODO:
- update docs
- migration
@leobragaz
added migration + test
ab007e3 
fixed bugs
@leobragaz leobragaz added kind/enhancement Enhance an already existing feature; no "New feature" to add x/posts Post module labels May 12, 2020
@leobragaz leobragaz added this to the v0.6.0 milestone May 12, 2020
@leobragaz leobragaz self-assigned this May 12, 2020
leobragaz
leobragaz commented May 12, 2020
View reviewed changes
Copy link
Contributor Author

@leobragaz leobragaz left a comment

Choose a reason for hiding this comment

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

re-reviewed

@leobragaz leobragaz requested a review from RiccardoM May 12, 2020 16:28
RiccardoM
RiccardoM suggested changes May 13, 2020
View reviewed changes
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

The changes looks overall good to me, however I would change just two small things.

CHANGELOG.md Show resolved Hide resolved
x/posts/internal/keeper/querier.go Outdated
} else {
registeredReaction, _ := keeper.GetRegisteredReaction(ctx, reaction.Value, post.Subspace)
reactionValue = registeredReaction.Value
}
reactionsResponses[index] = types.ReactionQueryResponse{
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the new PostReaction type is pretty much equal to the ReactionQueryResponse type, I think that we can delete the latter and leave only the PostReaction type to be used inside the PostQueryResponse object.

@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #160 into master will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
+ Coverage   82.83%   83.13%   +0.30%     
==========================================
  Files          57       58       +1     
  Lines        2423     2449      +26     
==========================================
+ Hits         2007     2036      +29     
+ Misses        357      356       -1     
+ Partials       59       57       -2

@leobragaz leobragaz requested a review from RiccardoM May 13, 2020 09:13
@leobragaz
Merge remote-tracking branch 'origin/leonardo/post-reaction-struct-im…
da7cc5a 
…prov' into leonardo/post-reaction-struct-improv
RiccardoM
RiccardoM suggested changes May 13, 2020
View reviewed changes
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

There are some unwanted changes in the tests

x/posts/internal/types/models/post_response_test.go Outdated Show resolved Hide resolved
x/posts/internal/types/models/post_response_test.go Outdated Show resolved Hide resolved
x/posts/internal/types/models/post_response_test.go Outdated Show resolved Hide resolved
x/posts/internal/types/models/post_response_test.go Outdated Show resolved Hide resolved
x/posts/internal/types/models/post_response_test.go Outdated Show resolved Hide resolved
x/posts/internal/types/models/post_response_test.go Outdated Show resolved Hide resolved
x/posts/internal/types/models/post_response_test.go Outdated
Comment on lines 180 to 182
expected := `ID: dd065b70feb810a8c6f535cf670fe6e3534085221fa964ed2660ebca93f910d1
Reactions: [{"owner":"cosmos1s3nh6tafl4amaxkke9kdejhp09lk93g9ev39r4","shortcode":"https://example.com/like","value":":like:"} {"owner":"cosmos15lt0mflt6j9a9auj7yl3p20xec4xvljge0zhae","shortcode":"👍","value":":+1:"}]
Children: [dd065b70feb810a8c6f535cf670fe6e3534085221fa964ed2660ebca93f910d1, dd065b70feb810a8c6f535cf670fe6e3534085221fa964ed2660ebca93f910d1]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert these lines as they should not change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

everything should be fine now, there was a problem inside the PostReaction type where its fields are alfabetically ordered (probably due to a refactor that I've done).

Copy link
Contributor

Choose a reason for hiding this comment

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

There is still something fishy about the tests. You are creating the reactions as follows:

[]models.PostReaction{
  models.NewPostReaction(":like:", "https://example.com/like", liker),
  models.NewPostReaction(":+1:", "👍", otherLiker),
},

But then you are testing if the result is

[
  {
    "owner": "cosmos1s3nh6tafl4amaxkke9kdejhp09lk93g9ev39r4",
    "shortcode": "https://example.com/like",
    "value": ":like:"
  }
  {
    "owner": "cosmos15lt0mflt6j9a9auj7yl3p20xec4xvljge0zhae",
    "shortcode": "👍",
    "value": ":+1:"
  }
]

As you can see in the result the value and shortcode values are inverted. There is either an error in the test or an error somewhere else. Please make the test expected result correct (invert those values) and fix the bug accordingly.

Copy link
Contributor Author

@leobragaz leobragaz May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yeah that's because I've not commit the changes yet

leobragaz and others added 6 commits May 13, 2020 15:28
@leobragaz @RiccardoM
Update x/posts/internal/types/models/post_response_test.go
e9e8b50 
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@leobragaz @RiccardoM
Update x/posts/internal/types/models/post_response_test.go
9e36493 
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@leobragaz @RiccardoM
Update x/posts/internal/types/models/post_response_test.go
770720d 
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@leobragaz @RiccardoM
Update x/posts/internal/types/models/post_response_test.go
6417fea 
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@leobragaz @RiccardoM
Update x/posts/internal/types/models/post_response_test.go
b759a4c 
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@leobragaz @RiccardoM
Update x/posts/internal/types/models/post_response_test.go
4dd14ef 
Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
RiccardoM
RiccardoM suggested changes May 13, 2020
View reviewed changes
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

Still a wrong test expected result

x/posts/internal/types/models/post_response_test.go Outdated
Comment on lines 180 to 182
expected := `ID: dd065b70feb810a8c6f535cf670fe6e3534085221fa964ed2660ebca93f910d1
Reactions: [{"owner":"cosmos1s3nh6tafl4amaxkke9kdejhp09lk93g9ev39r4","shortcode":"https://example.com/like","value":":like:"} {"owner":"cosmos15lt0mflt6j9a9auj7yl3p20xec4xvljge0zhae","shortcode":"👍","value":":+1:"}]
Children: [dd065b70feb810a8c6f535cf670fe6e3534085221fa964ed2660ebca93f910d1, dd065b70feb810a8c6f535cf670fe6e3534085221fa964ed2660ebca93f910d1]`
Copy link
Contributor

Choose a reason for hiding this comment

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

There is still something fishy about the tests. You are creating the reactions as follows:

[]models.PostReaction{
  models.NewPostReaction(":like:", "https://example.com/like", liker),
  models.NewPostReaction(":+1:", "👍", otherLiker),
},

But then you are testing if the result is

[
  {
    "owner": "cosmos1s3nh6tafl4amaxkke9kdejhp09lk93g9ev39r4",
    "shortcode": "https://example.com/like",
    "value": ":like:"
  }
  {
    "owner": "cosmos15lt0mflt6j9a9auj7yl3p20xec4xvljge0zhae",
    "shortcode": "👍",
    "value": ":+1:"
  }
]

As you can see in the result the value and shortcode values are inverted. There is either an error in the test or an error somewhere else. Please make the test expected result correct (invert those values) and fix the bug accordingly.

@leobragaz
fixed suggestions
d9b5185 
fixed a bug inside IndexOfByUserAndValue
@leobragaz leobragaz requested a review from RiccardoM May 13, 2020 14:36
RiccardoM
RiccardoM approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

LGTM

@RiccardoM RiccardoM merged commit b662341 into master May 13, 2020
@RiccardoM RiccardoM deleted the leonardo/post-reaction-struct-improv branch May 13, 2020 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RiccardoM RiccardoM RiccardoM approved these changes

Assignees

@leobragaz leobragaz

Labels
kind/enhancement Enhance an already existing feature; no "New feature" to add x/posts Post module
Projects
None yet
Milestone
v0.6.0
Development

Successfully merging this pull request may close these issues.

Improve the structure of PostReaction
2 participants
@leobragaz @RiccardoM