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

NT-953: Fetch a thread query #1266

Merged
merged 9 commits into from Jun 1, 2021
Merged

Conversation

Arkariang
Copy link
Contributor

📲 What

  • Create new method for the GraphQL networking client able to fetch all the replies to a concrete comment

🤔 Why

  • We will load the Thread screen with the list of replies to a concrete comment

🛠 How

  • Added new replies.graphql file, note I'm reusing all the fragments previously created :)
  • apolloClient has a new method getRepliesForComment that executes previous mentioned query and retruns a CommentEnvelope
  • On ThreadViewModel there is a subscription already working that fetches replies, for now it just displays on logcat the retrieved data

Screen Shot 2021-05-28 at 3 47 38 PM

👀 See

  • On Logcat you can see the CommentEnvelope Structure

LogcatReplies

  • it matches the comments on the web
    RepliesOnWeb

  • You can also take a look on the networking profiler

ProfilerResponse

| | |

📋 QA

  • Go to any comment with replies, and check logcat on your environment, you should see the message showing the comment envelope content

Story 📖

NT-1953

@@ -105,7 +105,6 @@ android.applicationVariants.all { variant ->

android {
compileSdkVersion 30
buildToolsVersion '29.0.3'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

buildToolsVersion no need to be specified any more, the gradle pluging now use their own versions

@@ -189,44 +189,6 @@ class KSApolloClient(val service: ApolloClient) : ApolloClientType {
}
}

private fun createCommentObject(commentFr: fragment.Comment?): Comment {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been moved outside the KSApollo class as a private package method nothing more, I was perfectly able to use it for replies as well 👍

.build()
}

private fun createPageInfoObject(pageFr: fragment.PageInfo?): PageInfoEnvelope {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been moved outside the class as a private package method nothing more, I was perfectly able to use it for replies as well 👍

@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #1266 (93b8eed) into master (f9637d1) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1266      +/-   ##
============================================
- Coverage     74.36%   74.34%   -0.02%     
+ Complexity      740      739       -1     
============================================
  Files           221      221              
  Lines          6646     6646              
  Branches        405      405              
============================================
- Hits           4942     4941       -1     
  Misses         1569     1569              
- Partials        135      136       +1     
Impacted Files Coverage Δ
.../src/main/java/com/kickstarter/models/Comment.java 60.00% <100.00%> (-6.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9637d1...93b8eed. Read the comment docs.

@Arkariang Arkariang marked this pull request as ready for review May 28, 2021 22:59
@Arkariang
Copy link
Contributor Author

No unit test added yet as this task was specific to create the query only and we have not specified any output yet, once the CommentEnvelope or the list of comments becomes an output we'll create the tests

Copy link
Contributor

@sunday-okpoluaefe sunday-okpoluaefe left a comment

Choose a reason for hiding this comment

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

lgtm!

@@ -36,6 +36,8 @@ interface ApolloClientType {

fun getProjectComments(slug: String, cursor: String?, limit: Int = 25): Observable<CommentEnvelope>

fun getRepliesForComment(comment: Comment, cursor: String = "", pageSize: Int = 25): Observable<CommentEnvelope>
Copy link
Contributor

Choose a reason for hiding this comment

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

Great job @Arkariang . Just tiny comment; can we rename pageSize to limit or limit to pageSize for consistency?. Otherwise looks good

Copy link
Contributor

Choose a reason for hiding this comment

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

We have const on my brach for page size we can use it after merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Arkariang Arkariang merged commit 3ad9ab3 into master Jun 1, 2021
@Arkariang Arkariang deleted the imartin/NT-1953-fetch-a-thread branch June 1, 2021 16:40
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.

None yet

3 participants