Rework Authentication #180

MaximeZmt · 2022-04-15T20:29:02Z

Rework Authentication (part 1)

This is related to issue #161

This Pull Requests contains:

Changing the welcome screen logic: if not logged in, write it to the user and when click on my account bring to authentication activity
If was logged in, when click on my account lead to profile activity.
Add of return button inside profile activity (to be extended to others)

In addition the code is tested with a coverage of: [??]%

Notes for reviewers

GOAL: Remove shared pref cause very buggy, will use an offline feature of firebase. Has to rework the whole authentication + storage process => Use Firebase authentication + database cache

=> I have read many docs concerning firebase, I've already spent more than 10h doing all my stuff.
Here is the first pr for the first part, the 2nd one will come later in the week.

laurislopata · 2022-04-18T09:03:40Z

app/src/main/java/ch/sdp/vibester/database/Database.kt

@@ -10,6 +10,7 @@ import com.google.firebase.ktx.Firebase

 object Database {
    fun get(): FirebaseDatabase {
-        return Firebase.database("https://vibester-sdp-default-rtdb.europe-west1.firebasedatabase.app")
+        val db = Firebase.database("https://vibester-sdp-default-rtdb.europe-west1.firebasedatabase.app")
+        return db


do we need the extra variable declaration here?

no that was temporary

laurislopata

LGTM already just one small comment

kamilababayeva

Thank you for your changes! There are several issues I ave addressed in the comments, can you respond to them? Thanks!

General comment, we should redirect user to the profile account after the user created the account. Right now, the user created account, automatically logged in, and then it is not redirected to profile account. You press go back, and the login is not displayed in the bottom of welcome page. It is resetted apparently.

kamilababayeva · 2022-04-18T08:54:54Z

app/src/main/java/ch/sdp/vibester/database/UsersRepo.kt

@@ -76,11 +76,15 @@ class UsersRepo @Inject constructor() {
                for (dataSnapShot in dataSnapshot.children) {
                    val dbContents = dataSnapShot.getValue<UserProfile>()
                    if (dbContents != null) {
+                        Log.e("pp urepo ",dbContents.image )
+                        Log.e("memail 1 ",dbContents.email )
+                        Log.e("memail 2 ",email )


in general, is it okay that log is there?

I am asking cause they dont respond to the error, so they are for personal testing. From the 'best' practices, should they be in the main branch of the code? If yes, then lets do more comprehensive text?

no that was debug stuff and will be removed

kamilababayeva · 2022-04-18T08:55:15Z

app/src/main/java/ch/sdp/vibester/database/UsersRepo.kt

                            callback(dbContents)
                            break
                        } else {
-                            callback(UserProfile())
+                            //callback(UserProfile())


should we remove this callback?

yes this callback is wrong, cause it goes across around all player in db, and validate the login even if not the good one

I will even remove the comments and clear the line

kamilababayeva · 2022-04-18T09:11:22Z

app/src/main/java/ch/sdp/vibester/activity/WelcomeActivity.kt

+        /*
+        val getIntent = intent.extras
+        if (getIntent!=null){
+            testLoggedIn = getIntent.getBoolean("testLoggedIn", false)
        }



if not used, remove completely?

yes (sorry old test stuff)

kamilababayeva · 2022-04-18T09:13:07Z

app/src/main/java/ch/sdp/vibester/activity/WelcomeActivity.kt

+
+
+        val userStatusTextValue = findViewById<TextView>(R.id.user_status)
+        if(FireBaseAuthenticator.isLoggedIn() ||  testLoggedIn)


Since we are pushing to the main branch, I believe the code should be as clean as possible. So I am not sure whether the testing variables hsould be in main

this is testing var for cirrus, this is necessary (trick to allow unit test to test the below code, otherwise impossible)

ohhh, i seee

kamilababayeva · 2022-04-18T09:15:48Z

app/src/main/java/ch/sdp/vibester/activity/ProfileActivity.kt

+            if (bm != null) {
+                Log.e(getString(R.string.log_tag), bm.height.toString() + " - " + bm.width.toString())
+            }else{
+                Log.e(getString(R.string.log_tag), "ahhhh merde")


noooo no merde

hahahaha forget to remove it 😅 😅 Do we see I get frustrated with that code 😂 ?

kamilababayeva · 2022-04-18T09:19:40Z

app/src/main/java/ch/sdp/vibester/activity/ProfileActivity.kt

-                    bit.get()
+                    Log.e(getString(R.string.log_tag),user.image)
+                    val bit = BitmapGetterApi.download("https://raw.githubusercontent.com/Ashwinvalento/cartoon-avatar/master/lib/images/male/45.png")
+                    bit.get(10, TimeUnit.SECONDS)


the user image will be displayed during 10 seconds? isnt it a lot?

No the 10 sec is the timeout to get the image. Indeed it is a magic number for the moment, but I may set it a global var for all async timeout and should choose the value later

kamilababayeva · 2022-04-18T09:49:47Z

app/src/main/java/ch/sdp/vibester/activity/ProfileActivity.kt

-                    val bit = BitmapGetterApi.download("https://" + user.image)
-                    bit.get()
+                    Log.e(getString(R.string.log_tag),user.image)
+                    val bit = BitmapGetterApi.download("https://raw.githubusercontent.com/Ashwinvalento/cartoon-avatar/master/lib/images/male/45.png")


something to think about...

There are three options about profile images:

We save the default image link in every profile in firebase.

We save an image in our app

We upload the picture from github every time (as you do)

We use profile picture in scoreboard, user search and in a profile account. If we take scoreboard, there might be a lot of profiles without the image, maybe it would be better if each of the profile has a default image, so we dont do extra check and upload image your way. Or it would be better to have an image locally, since it is same and we dont upload your way every time.

yes we will see and try to solve this issue. Currently this is just a random image to test the display. I will rework it in part 2. There was issue with the database and account code that I will solve in part 2.

MaximeZmt · 2022-04-18T17:31:14Z

Thank you for your changes! There are several issues I ave addressed in the comments, can you respond to them? Thanks!

General comment, we should redirect user to the profile account after the user created the account. Right now, the user created account, automatically logged in, and then it is not redirected to profile account. You press go back, and the login is not displayed in the bottom of welcome page. It is resetted apparently.

Yes It will be done in the part 2 the following week, it is part of the rework

codeclimate · 2022-04-18T17:55:38Z

Code Climate has analyzed commit d45b7df and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

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

This pull request will bring the total coverage in the repository to 87.9% (-0.8% change).

View more on Code Climate.

kamilababayeva · 2022-04-18T20:44:01Z

Thank you for replying to my comments! Approved!

doing debug

475483a

MaximeZmt self-assigned this Apr 15, 2022

begin rework auth

8460d2c

MaximeZmt changed the title ~~doing debug~~ Rework Apr 16, 2022

MaximeZmt added 4 commits April 17, 2022 12:55

currently update UI and user path

43a062e

update

6e2920a

merged done

41eb382

modified authentication +welcome screen

f2ba5d9

MaximeZmt changed the title ~~Rework~~ Rework Authentication Rework Apr 17, 2022

MaximeZmt changed the title ~~Rework Authentication Rework~~ Rework Authentication Apr 17, 2022

correctionTest

55e7377

MaximeZmt added the actual: 10h actual time to fix label Apr 17, 2022

MaximeZmt marked this pull request as ready for review April 18, 2022 03:49

laurislopata reviewed Apr 18, 2022

View reviewed changes

kamilababayeva self-requested a review April 18, 2022 09:03

laurislopata approved these changes Apr 18, 2022

View reviewed changes

kamilababayeva suggested changes Apr 18, 2022

View reviewed changes

corrected pr

d45b7df

MaximeZmt requested a review from kamilababayeva April 18, 2022 18:03

kamilababayeva approved these changes Apr 18, 2022

View reviewed changes

MaximeZmt merged commit 40104f0 into main Apr 18, 2022

MaximeZmt deleted the maximezmt/fixpref branch April 18, 2022 20:47

MaximeZmt linked an issue Apr 18, 2022 that may be closed by this pull request

Part 1 of Update Score + Refactor Fix account #188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Authentication #180

Rework Authentication #180

MaximeZmt commented Apr 15, 2022 •

edited

Loading

laurislopata Apr 18, 2022

MaximeZmt Apr 18, 2022

laurislopata left a comment

kamilababayeva left a comment

kamilababayeva Apr 18, 2022

kamilababayeva Apr 18, 2022

MaximeZmt Apr 18, 2022

kamilababayeva Apr 18, 2022

MaximeZmt Apr 18, 2022

MaximeZmt Apr 18, 2022

kamilababayeva Apr 18, 2022

MaximeZmt Apr 18, 2022

kamilababayeva Apr 18, 2022

MaximeZmt Apr 18, 2022

kamilababayeva Apr 18, 2022

kamilababayeva Apr 18, 2022

MaximeZmt Apr 18, 2022

kamilababayeva Apr 18, 2022

kamilababayeva Apr 18, 2022

MaximeZmt Apr 18, 2022

kamilababayeva Apr 18, 2022

kamilababayeva Apr 18, 2022

MaximeZmt Apr 18, 2022

MaximeZmt commented Apr 18, 2022

codeclimate bot commented Apr 18, 2022

kamilababayeva commented Apr 18, 2022



		val userStatusTextValue = findViewById<TextView>(R.id.user_status)
		if(FireBaseAuthenticator.isLoggedIn() \|\| testLoggedIn)

Rework Authentication #180

Rework Authentication #180

Conversation

MaximeZmt commented Apr 15, 2022 • edited Loading

Rework Authentication (part 1)

This is related to issue #161

Notes for reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurislopata left a comment

Choose a reason for hiding this comment

kamilababayeva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximeZmt commented Apr 18, 2022

codeclimate bot commented Apr 18, 2022

kamilababayeva commented Apr 18, 2022

MaximeZmt commented Apr 15, 2022 •

edited

Loading