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

Refactor Auth + Going forward score part 2 #190

Merged
merged 51 commits into from
Apr 28, 2022
Merged

Conversation

MaximeZmt
Copy link
Owner

@MaximeZmt MaximeZmt commented Apr 19, 2022

Refactor Authentication Process + Updates Score into Firebase

This is related to issue #161

This Pull Requests contains:

  • Creation of user is fail safe. If the creation of user is successful, it will assign the user automatically to the linked part in the database. (before was needed another activity that if the user leave, it will block and loose it's account access)
  • Major refactor of google SignIn and LogIn: Now creates an account if first time log in, then manage as a normal user
  • Adding return Button in the game setup activity. Allow if missclick to return to the previous setting screen
  • Change the User UID, now uses the firebase auth UID before was using a random generated one -> much better for further dev in the app and easier
  • Added method in FireBaseAuthenticator to check account status and get information
  • Set Persistance of Database. It means that once the app has been open and loaded, as long as the user does not close it, even if he/she looses connection, the score will be still updated in the firebase as soon as he/she recover connection.
  • Has created method to update score in the database (set, relative update, best score)
  • Added a TestMode static class that helps to keep track in certain method whether is has been launched by unit test or not. (useful to bypass some verification like auth in unit test to test some button features, ...)

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

Notes for reviewers

@MaximeZmt MaximeZmt self-assigned this Apr 19, 2022
createAcc: Boolean,

//TODO
private fun startNewActivity(email: String) {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@MaximeZmt MaximeZmt marked this pull request as ready for review April 27, 2022 13:37
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.

review in progress

app/src/main/java/ch/sdp/vibester/TestMode.kt Show resolved Hide resolved
app/src/main/java/ch/sdp/vibester/api/InternetState.kt Outdated Show resolved Hide resolved
app/src/main/java/ch/sdp/vibester/api/InternetState.kt Outdated Show resolved Hide resolved
* @param data intent returned from google sign in
*/
fun googleActivityResult(requestCode: Int, resultCode: Int, data: Intent?, ctx: Context): String? {
if (requestCode == 1000) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

request code 1000 appears more than once in different files, it might be useful to store it as a const and give it an appropriate name.

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.

Thanks for this BIG refactoring/implementation.
As discussed, test coverage will be improved later.
What do you think of the idea of breaking down a huge PR into smaller PRs
and giving more meaningful commit messages?

Copy link
Collaborator

@kamilababayeva kamilababayeva left a comment

Choose a reason for hiding this comment

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

I have left a partial review, I will look at your pr again after the changes. Lets do several PRs in next sprints, please!

But thank you a lot for your changes. They are nice!!

@kamilababayeva kamilababayeva self-requested a review April 28, 2022 15:23
@@ -17,12 +17,16 @@ import ch.sdp.vibester.R
import ch.sdp.vibester.api.LastfmMethod
import ch.sdp.vibester.helper.TypingGameManager
import org.hamcrest.CoreMatchers.not
import org.junit.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good change!

Copy link
Collaborator

@kamilababayeva kamilababayeva left a comment

Choose a reason for hiding this comment

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

What causes such a drop in the tests coverage?

I gave it a second quick look, I have some questions. Next review I will approve, I promise!!!

private var hasAlreadyAccessedInternetOnce: Boolean = false
fun hasAccessedInternetOnce(ctx: Context):Boolean{
if (!hasAlreadyAccessedInternetOnce && getInternetStatus(ctx)) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra unnecessary line

Copy link
Collaborator

@laurislopata laurislopata left a comment

Choose a reason for hiding this comment

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

Good job!

@kamilababayeva kamilababayeva self-requested a review April 28, 2022 17:00
Copy link
Collaborator

@kamilababayeva kamilababayeva left a comment

Choose a reason for hiding this comment

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

Thank you for your work! The changes are nice and useful, you put a loooot of effort 💟 🎧

I hope we can separate big tasks in smaller tasks/prs next time. It is hard to check, and I believe it causes more errors/wrong decisions. go rest!

@codeclimate
Copy link

codeclimate bot commented Apr 28, 2022

Code Climate has analyzed commit 8e559fc and detected 14 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 4
Duplication 10

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

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

View more on Code Climate.

@MaximeZmt MaximeZmt merged commit 7916ffc into main Apr 28, 2022
@MaximeZmt MaximeZmt deleted the maximezmt/refactorauthp2 branch April 28, 2022 18:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants