-
Notifications
You must be signed in to change notification settings - Fork 527
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
Fix #4803 Failed to add Admin (or non-admin) Profile Picture. #5118
Fix #4803 Failed to add Admin (or non-admin) Profile Picture. #5118
Conversation
Thanks @ShubhadeepKarmakar! Hi @kkmurerwa, could you give this a pass and assign it to me once any initial concerns have been addressed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ShubhadeepKarmakar!
In addition to the tests, please update the following so that we can merge your PR:
- Your PR title should either describe the problem being solved, or make it the same as the issue title.
- Instead of saying "Added two lines to the codebase and resolved.", please explain the change you have made in terms of why the old code didn't work, and why your change fixes the bug. This is helpful for future developers reading the code.
app/src/main/java/org/oppia/android/app/profileprogress/ProfileProgressActivityPresenter.kt
Show resolved
Hide resolved
Hi @ShubhadeepKarmakar, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks! |
Hi @adhiamboperes. I see that you have already done a review of this PR. I don't think I have any further comments. @ShubhadeepKarmakar if you get stuck, feel free to reach out to me. |
@kkmurerwa I am facing this problem at the time of updating the test cases |
Hi @ShubhadeepKarmakar. The problem could be the Android Studio version that you are using. Please check that you are using Android Studio Bumblebee. |
I'm currently using Android Studio Electric Eel, |
…ar/oppia-android into profile_picture for pushing the code into forked repo
My experience with Xiaomi devices is that you need to accept the install permission request. It's best to use the "remember my choice" option. An alternative is to setup an Android emulator. Preferably pixel 3a sdk 29. Could you also take another look at the wiki set up instructions for additional information? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I just had one more comment, but otherwise your changes look good so far.
app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfileProgressFragmentTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ShubhadeepKarmakar!
Congratulations on getting your first PR approveed! Your changes LGTM.
Hi @ShubhadeepKarmakar, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
Explanation
val galleryIntent = Intent(Intent.ACTION_PICK, MediaStore.Images.Media.EXTERNAL_CONTENT_URI)
The above code (previous) is the explicit way to get images from gallery but is not working here. But the updated code below is less explicit but works fine,
val galleryIntent = Intent(Intent.ACTION_GET_CONTENT).apply { type = "image/*" }
Essential Checklist
Screen Recording
oppia.mp4