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

Fixes #3389 - Show User profiles #4678

Merged
merged 2 commits into from Oct 25, 2021

Conversation

ashishkumar468
Copy link
Collaborator

**Description (required)
Fixes #3389
**What changes did you make and why?
Profile Activity, AchievementsFragment, ContributionsFragment & LeaderboardFragment accept username instead of using the default account.
Added RemoteDataSource for contributions that do not persist contributions.
On Clicks on AuthorNames in MediaDetails, Menu-Options in MediaDetails & Leaderboard list takes to the profile of that user with the corresponding Achievements, Leaderboard and ContributionsList.

Tests performed (required)

Tested betaDebug/prodDebug on API 29

Screenshots (for UI changes only)

---image

image- image- image-
image

@neslihanturan
Copy link
Collaborator

Some tests fails with this PR @ashishkumar468 :(

@ashishkumar468
Copy link
Collaborator Author

Hey @neslihanturan , if you mean the integration tests, yes I am working on the fix, meanwhile would be great to have the code reviewed

Copy link
Collaborator

@neslihanturan neslihanturan left a comment

Choose a reason for hiding this comment

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

Hey @ashishkumar468 , the PR works great in general and I loved this new feature. Thanks for the great job! The only problem that I recignized is that previous user's numbers are being shown while the numbers (num of uploads, percentage of undeleted uploads etc.) are being fetched for selected user. This can make user think that numbers of the selected user are numbers of previously checked user. There should be a progressbar or at least they should be empty during this time. For example for level, we have progresbar and this prevents ambiguity.

@ashishkumar468
Copy link
Collaborator Author

Hey @neslihanturan , thanks for the quick review. Totally makes sense, what I am planning to do is probably keep them empty while the data is being fetched, would not make sense to show individual (and multiple progress bars) as the API calls are more than one.

@neslihanturan
Copy link
Collaborator

Hey @neslihanturan , thanks for the quick review. Totally makes sense, what I am planning to do is probably keep them empty while the data is being fetched, would not make sense to show individual (and multiple progress bars) as the API calls are more than one.

I think empty fields are better than having multiple progressbars

@ashishkumar468
Copy link
Collaborator Author

Hi @neslihanturan , I have made the suggested changes and the PR is up for review again

@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #4678 (92b4add) into master (6649da8) will decrease coverage by 0.18%.
The diff coverage is 36.19%.

❗ Current head 92b4add differs from pull request most recent head f0dc195. Consider uploading reports for the commit f0dc195 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4678      +/-   ##
============================================
- Coverage     38.76%   38.58%   -0.19%     
- Complexity     1666     1671       +5     
============================================
  Files           353      354       +1     
  Lines         14728    14845     +117     
  Branches       1296     1308      +12     
============================================
+ Hits           5710     5728      +18     
- Misses         8619     8712      +93     
- Partials        399      405       +6     
Impacted Files Coverage Δ
...ntributions/WikipediaInstructionsDialogFragment.kt 14.28% <0.00%> (-3.37%) ⬇️
...fr/free/nrw/commons/media/MediaDetailFragment.java 28.59% <0.00%> (-0.26%) ⬇️
...ee/nrw/commons/media/MediaDetailPagerFragment.java 24.87% <0.00%> (-0.63%) ⬇️
...mons/contributions/ContributionsListPresenter.java 39.39% <6.66%> (-20.61%) ⬇️
...ons/contributions/ContributionsRemoteDataSource.kt 17.85% <17.85%> (ø)
...ns/profile/leaderboard/LeaderboardListAdapter.java 44.44% <20.00%> (-5.56%) ⬇️
...mons/contributions/ContributionBoundaryCallback.kt 82.50% <25.00%> (-6.69%) ⬇️
...mmons/contributions/ContributionsListFragment.java 59.17% <33.33%> (-1.73%) ⬇️
...a/fr/free/nrw/commons/profile/ProfileActivity.java 82.29% <40.00%> (-14.97%) ⬇️
...mmons/profile/leaderboard/LeaderboardFragment.java 60.36% <40.00%> (-3.20%) ⬇️
... and 8 more

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 6649da8...f0dc195. Read the comment docs.

Copy link
Collaborator

@neslihanturan neslihanturan left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ashishkumar468 :)

@neslihanturan neslihanturan merged commit 88b21a6 into commons-app:master Oct 25, 2021
@@ -527,7 +527,8 @@ Upload your first media by tapping on the add button.</string>
<string name="coordinates_picking_unsuccessful">Unable to get coordinates.</string>

<string name="share_image_via">Share image via</string>
<string name="no_achievements_yet">You haven\'t made any contributions yet</string>
<string name="you_have_no_achievements_yet">You haven\'t made any contributions yet</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is far from ideal, as this this shows up as change of content + new message instead of rename + new message. You will be getting build failures since the translations will not contain %s until updated.

Ref: https://translatewiki.net/wiki/Special:ManageMessageGroups/non-mediawiki

I strongly recommend to use a different key when the contents are not compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User profiles
3 participants