-
Notifications
You must be signed in to change notification settings - Fork 0
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
Association Utils #60
Conversation
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.
Overall it looks good to me. However, I would suggest to add some comments especially inside some tests functions so that your code is easier to read and is more accessible.
import com.github.se.assocify.model.entities.User | ||
import kotlinx.coroutines.flow.MutableStateFlow | ||
|
||
class AssociationUtils( |
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.
Since it's a view model, wouldn't it be better to name this class AssociationModel or something else containing the word model in it?
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.
It is not a viewModel, I got confused. I used the functions I wrote for the wrong viewModel to create a class containing utility methods to create/modify/send requests to a certain association
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.
Okay, so I think you can remove the fact that AssociationUtils inherits from ViewModel()
import org.mockito.Mockito | ||
import org.mockito.Mockito.mock | ||
|
||
class AssociationUtilsTest { |
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.
(if you choose to change the name of your class don't forget to change the name of the test class to be consistent)
class AssociationUtilsTest { | ||
private lateinit var db: FirebaseFirestore | ||
private lateinit var assoApi: AssociationAPI | ||
private val documentSnapshot = Mockito.mock(DocumentSnapshot::class.java) |
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.
It seems that the usage of Mockito.mock might be unnecessary here. You could change the mock function calls by removing the Mockito prefix and simply using mock(...) instead.
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.
You're right gonna fix it asap
Mockito.`when`(documentSnapshot.toObject(Association::class.java)).thenReturn(oldAssoUpdated) | ||
Mockito.`when`(documentReference.get()).thenReturn(Tasks.forResult(documentSnapshot)) | ||
|
||
val task = Tasks.forResult(documentSnapshot) |
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.
This variable is never used you can safely delete it.
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.
Thank you!!
Quality Gate passedIssues Measures |
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.
LGTM! You can merge !
An Util class to do various operations on an association and to modify the association values on the database.
The class has as objective to solve some problems that will probably come often when dealing with associations, like accepting a new member, asking to enter in the association as a new member or get some elements of the Association.
Here are some functionalities we may want from it, and the advancement of them: