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

Linking profile to image upload #228

Merged
merged 21 commits into from
May 5, 2022

Conversation

laurislopata
Copy link
Collaborator

@laurislopata laurislopata commented May 2, 2022

This PR links the image upload from creating a user account to displaying it on the profile. This is a small PR but it took me longer than expected due to CI issues. The coverage is low because of database calls and image download functions so can't do much about it and it doesn't affect the overall coverage much

@laurislopata laurislopata marked this pull request as ready for review May 5, 2022 06:12
@codeclimate
Copy link

codeclimate bot commented May 5, 2022

Code Climate has analyzed commit f9c9282 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 54.8% (80% is the threshold).

This pull request will bring the total coverage in the repository to 84.0% (0.0% change).

View more on Code Climate.

@MaximeZmt MaximeZmt self-requested a review May 5, 2022 06:42
Copy link
Owner

@MaximeZmt MaximeZmt 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. Has just left some comments below.


val scn: ActivityScenario<ProfileActivity> = ActivityScenario.launch(intent)

Thread.sleep(5000)
Copy link
Owner

Choose a reason for hiding this comment

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

Why not use Completable Future to detect if images have been downloaded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, I'll implement it next week

app/src/main/java/ch/sdp/vibester/database/ImageGetter.kt Outdated Show resolved Hide resolved
app/src/main/java/ch/sdp/vibester/database/ImageGetter.kt Outdated Show resolved Hide resolved
app/src/main/java/ch/sdp/vibester/database/ImageGetter.kt Outdated Show resolved Hide resolved
app/src/main/java/ch/sdp/vibester/database/ImageGetter.kt Outdated Show resolved Hide resolved
app/src/main/java/ch/sdp/vibester/database/ImageGetter.kt Outdated Show resolved Hide resolved
@jiabaow jiabaow self-requested a review May 5, 2022 07:07

if(bm != null){
val avatar = findViewById<ImageView>(R.id.avatar)
avatar.setImageBitmap(Bitmap.createScaledBitmap(bm, 1000,1000, false))
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe put 1000 in a meaningful val?

val bit = BitmapGetterApi.download(imageURI.toString())
bit.get(10, TimeUnit.SECONDS)
}catch (e: Exception){
null
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the handle exception is null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaximeZmt wrote this function I just reused it, so maybe he can answer better :)

Copy link
Owner

Choose a reason for hiding this comment

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

It as was null because it was not a critical feature. I mean that the download of an image can fail for many reasons (user, server, Internet Provider, other users, ...)
Therefore if it does not works, As it was implemented at the beginning of the semester I've put null not knowing how to handle it.
Maybe there is a better way to do it now.

* @param imageID ID of the image in the database
* @param callback function to be called when image fetched
*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

@@ -184,6 +185,10 @@ class DataGetter @Inject constructor() {
dbRoomRef.child(partyRoom.getRoomID()).child("emailList").setValue(partyRoom.getEmailList())
}

fun getCurrentUser(): FirebaseUser? {
return authenticator.getCurrUser()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it in dataFetter? We need to think of a separation between firebaseUser and Datagetter

Copy link
Owner

@MaximeZmt MaximeZmt left a comment

Choose a reason for hiding this comment

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

👍

@@ -207,6 +208,10 @@ class DataGetter @Inject constructor() {
dbRoomRef.child(partyRoom.getRoomID()).child("emailList").setValue(partyRoom.getEmailList())
}

fun getCurrentUser(): FirebaseUser? {
Copy link
Collaborator

@Tsathogguaa Tsathogguaa May 5, 2022

Choose a reason for hiding this comment

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

Reminder to add documentation for functions that are not private, no matter how obvious they can be. Not too important for this PR as we need a general check over our entire codebase and write docs, but a small reminder

Copy link
Collaborator

@Tsathogguaa Tsathogguaa left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link
Collaborator

@jiabaow jiabaow left a comment

Choose a reason for hiding this comment

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

LGTM

@laurislopata laurislopata merged commit d68b1f5 into main May 5, 2022
@laurislopata laurislopata deleted the laurislopata/link-profile-to-imageUpload branch May 5, 2022 21:49
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.

5 participants