Skip to content

feat(model/event): Implement event repository #23

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

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

armouldr
Copy link
Collaborator

@armouldr armouldr commented Oct 8, 2024

Add event repository and event data type

Add an event repository and an event data class, to get and add data to the database
@armouldr armouldr force-pushed the feature/event-repository-model branch from 74a64c9 to 5eb1e54 Compare October 8, 2024 15:13
@armouldr armouldr marked this pull request as ready for review October 8, 2024 15:58
fun getNewUid(): String

fun addEvent(event: Event, onSuccess: () -> Unit, onFailure: (Exception) -> Unit)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to ask to the coaches about the updateEvent function: is it really necessary to write one in the repository since it is essentially a copy of the addEvent function of the repository or not.


private fun hydrate(doc: DocumentSnapshot): Event? {

val event = doc.toObject(Event::class.java)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did not know this worked, nice

@@ -145,6 +146,7 @@ play-services-auth = { module = "com.google.android.gms:play-services-auth", ver
play-services-maps = { module = "com.google.android.gms:play-services-maps", version.ref = "playServicesMaps" }
robolectric = { module = "org.robolectric:robolectric", version.ref = "robolectric" }
test-core-ktx = { group = "androidx.test", name = "core-ktx", version.ref = "androidxCoreKtx" }
firebase-common-ktx = { group = "com.google.firebase", name = "firebase-common-ktx", version.ref = "firebaseCommonKtx" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should remove these dependencies as they are unused

@@ -171,6 +171,7 @@ dependencies {
implementation(libs.androidx.material3)
implementation(libs.androidx.navigation.compose)
implementation(platform(libs.androidx.compose.bom))
implementation(libs.firebase.common.ktx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should remove this dependency as it is unused

Copy link

sonarqubecloud bot commented Oct 8, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
74.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@armouldr armouldr merged commit 6777e4f into main Oct 8, 2024
1 of 2 checks passed
@armouldr armouldr deleted the feature/event-repository-model branch October 8, 2024 16:54
Copy link
Collaborator

@leagrieder leagrieder left a comment

Choose a reason for hiding this comment

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

PR Review

  • Logging in hydrate() Function: The hydrate() function currently returns null if the conversion fails. It would be beneficial to log the error, as this will make future debugging easier and help identify issues during runtime.

  • Description Improvement: The PR description is too brief. Providing more detail would be helpful. One line is not enough to describe the changes and their purpose. Here's an example template for improving descriptions: PR Template Example.

  • Positive Feedback: The code is clean and well-structured. Great job on implementing comprehensive testing for your event repository! The attention to testing is commendable. Keep up the good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants