Skip to content
This repository has been archived by the owner on May 31, 2019. It is now read-only.

Update comment schema #82

Merged
merged 3 commits into from
Sep 13, 2018
Merged

Update comment schema #82

merged 3 commits into from
Sep 13, 2018

Conversation

danielpassos
Copy link
Contributor

@danielpassos danielpassos commented Aug 30, 2018

This PR:

  • Remove comments from Meme (type and query)
  • Add a new Query (comments by memeid)
  • Add Owner relationship from Comment
  • Fix Owner relationship on Meme (from Array to a single type)
  • Fix responseMapping for all postgres querys
  • Probably something else I can't remember right now :bowtie:

@wtrocki
Copy link
Contributor

wtrocki commented Aug 31, 2018

@danielpassos Changes looks great. I have no specific comment, but it's best to hold off as we do not have this replicated for IOS.

@wtrocki
Copy link
Contributor

wtrocki commented Aug 31, 2018

Going to create ticket to do IOS update.

@wtrocki
Copy link
Contributor

wtrocki commented Sep 5, 2018

let's rebase it later and get that in after demo together with android updates.

@aerogear-attic aerogear-attic deleted a comment from codecov-io Sep 5, 2018
@wtrocki
Copy link
Contributor

wtrocki commented Sep 5, 2018

Good stuff. Do we need to udpate proposal?
Let me known when you will be happy for me to check that again

@aerogear-attic aerogear-attic deleted a comment from codecov-io Sep 5, 2018
@danielpassos
Copy link
Contributor Author

@wtrocki It's ready for review. I already have changed the Android and going to do the same on iOS.

@wtrocki
Copy link
Contributor

wtrocki commented Sep 7, 2018

Good to be merged. I want to push that to community cluster for wider tests

Copy link
Contributor

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

Once merged let's put that on community cluster so we will benefit from that on mobile clients.

@danielpassos
Copy link
Contributor Author

@darahayes Any thoughts? Should I merge it?

@darahayes
Copy link
Contributor

@danielpassos no problems with this PR. Just one question. Why remove comments from the meme type?

@danielpassos
Copy link
Contributor Author

danielpassos commented Sep 10, 2018

@darahayes From the memeolist app perspective:

  1. I don't know if the comment will be showed (you need to go to the comment screen), so why retrieve the comment with the meme and fire 1 comment select for each meme?
  2. When a new comment is added it will not be displayed in any app without reloading all memes because comments were in the meme type/query

@aerogear-attic aerogear-attic deleted a comment from codecov-io Sep 10, 2018
@aerogear-attic aerogear-attic deleted a comment from codecov-io Sep 10, 2018
@darahayes
Copy link
Contributor

@danielpassos makes perfect sense. LGTM

@danielpassos danielpassos merged commit 74041e1 into aerogear-attic:master Sep 13, 2018
@danielpassos danielpassos deleted the comment-query branch September 13, 2018 17:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants