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

Fix issue 3370 #3381

Merged
merged 24 commits into from
Aug 5, 2024
Merged

Fix issue 3370 #3381

merged 24 commits into from
Aug 5, 2024

Conversation

rodrigoliveirac
Copy link
Contributor

Pull request (PR) checklist

Please check if your pull request fulfills the following requirements:

  • I've read the Contribution Guidelines and my PR doesn't break the rules.
  • I've read and understand the Developer Guidelines.
  • I confirm that I've run the code locally and everything works as expected.
  • My PR includes only the necessary changes to fix the issue (i.e., no unnecessary files or lines of code are changed).
  • 🎬 I've attached a screen recording of using the new code to the next paragraph (if applicable).

Screen recording of interacting with your changes:

What's changed?

Describe with a few bullets what's new:

  • The issue was due to using a query that only fetched the maximum number of contributors specified by the per_page parameter. We are now also using the page parameter for pagination and including anonymous contributors with the anon parameter.
  • To include anonymous contributors, we needed to make the fields in the ContributorDto model nullable, with null as the default value.

Risk factors

What may go wrong if we merge your PR?

  • In the future, we should consider implementing paging in the UI to prevent users from waiting too long.

Does this PR close any GitHub issues? (do not delete)

Troubleshooting GitHub Actions (CI) failures ❌

Pull request checks failing? Read our CI Troubleshooting guide.

ILIYANGERMANOV
ILIYANGERMANOV requested changes Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Left some comments, add unit tests (if possible), fix the CI and we can merge it

screen/contributors/src/main/java/com/ivy/contributors/IvyWalletRepositoryDataSource.kt Outdated Show resolved Hide resolved
screen/contributors/src/main/java/com/ivy/contributors/IvyWalletRepositoryDataSource.kt Outdated Show resolved Hide resolved
screen/contributors/src/main/java/com/ivy/contributors/ContributorsViewModel.kt Show resolved Hide resolved
@ILIYANGERMANOV ILIYANGERMANOV added the requested changes Changes are needed. Please, apply them label Jul 31, 2024
ILIYANGERMANOV
ILIYANGERMANOV requested changes Aug 1, 2024
View reviewed changes
Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

Left some more comments. Thanks for migrating it to FP-style and Arrow!

buildSrc/src/main/kotlin/ivy.script.gradle.kts Show resolved Hide resolved
screen/contributors/src/main/java/com/ivy/contributors/ContributorsViewModel.kt Outdated Show resolved Hide resolved
screen/contributors/src/main/java/com/ivy/contributors/IvyWalletRepositoryDataSource.kt Outdated Show resolved Hide resolved
screen/contributors/src/test/java/com/ivy/contributors/ContributorsViewModelTest.kt Outdated Show resolved Hide resolved
screen/contributors/src/test/java/com/ivy/contributors/ContributorsViewModelTest.kt Outdated Show resolved Hide resolved
screen/contributors/src/main/java/com/ivy/contributors/IvyWalletRepositoryDataSource.kt Outdated Show resolved Hide resolved
screen/contributors/src/main/java/com/ivy/contributors/IvyWalletRepositoryDataSource.kt Outdated Show resolved Hide resolved
@rodrigoliveirac
Copy link
Contributor Author

Left some more comments. Thanks for migrating it to FP-style and Arrow!

My pleasure! I’ll study this concept further

@ILIYANGERMANOV
Copy link
Collaborator

@rodrigoliveirac also make sure to check this:

@rodrigoliveirac
Copy link
Contributor Author

@ILIYANGERMANOV To avoid unnecessary complexity, I’ve decided to leave this test unimplemented for now

ILIYANGERMANOV
ILIYANGERMANOV approved these changes Aug 2, 2024
View reviewed changes
Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

Left some comments about incorrect Arrow usages. Ping me to merge after they are resolved and the CI has passed

screen/contributors/src/main/java/com/ivy/contributors/IvyWalletRepositoryDataSource.kt Outdated Show resolved Hide resolved
screen/contributors/src/main/java/com/ivy/contributors/ContributorsViewModel.kt Outdated Show resolved Hide resolved
screen/contributors/src/main/java/com/ivy/contributors/IvyWalletRepositoryDataSource.kt Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Stale No activity, will be automatically closed soon. label Aug 5, 2024
ILIYANGERMANOV
ILIYANGERMANOV approved these changes Aug 5, 2024
View reviewed changes
Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

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

Nice work 👏 LGTM!

@ILIYANGERMANOV ILIYANGERMANOV merged commit c06b73a into Ivy-Apps:main Aug 5, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ILIYANGERMANOV ILIYANGERMANOV ILIYANGERMANOV approved these changes

Assignees
No one assigned
Labels
requested changes Changes are needed. Please, apply them Stale No activity, will be automatically closed soon.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Implement pagination for the "Contributors" screen
2 participants
@rodrigoliveirac @ILIYANGERMANOV