Fix user delete to properly delete user's relations in the app #309
Conversation
Romainhir
changed the title
Test failed when image was not found
…nclick Feature: maximise event picture when clicking on it
Redd87
left a comment
Redd87
left a comment
There was a problem hiding this comment.
An elegant solution to the problem! However you completely forgot to comment your new functions and classes, as well as the ones you modified previously. I'm also curious as to why you overwrite the Event hashCode and equals method.
As I think I might have overlooked some aspects I'd also like someone else to look at the code
| } | ||
| } | ||
|
|
||
| override fun deleteUser( |
There was a problem hiding this comment.
Please comment your new functions and classes
| */ | ||
| suspend fun deleteUser( | ||
| userId: String, | ||
| user: User, |
There was a problem hiding this comment.
And modify the corresponding javadoc alongside the function
| override fun equals(other: Any?): Boolean { | ||
| if (this === other) return true | ||
| if (javaClass != other?.javaClass) return false | ||
|
|
||
| other as Event | ||
|
|
||
| return uid == other.uid | ||
| } | ||
|
|
||
| override fun hashCode(): Int { | ||
| return uid.hashCode() | ||
| } | ||
|
|
There was a problem hiding this comment.
SonarCloud consider not implementing both equals and hashcode together as a maintanability issue, so that's why I implemented it
…nt-Group13/Unio into fix/delete-user-dependencies
|
Good job making the requested changes, I'd also just like someone else to have a look at the code as mentioned previously |
There was a problem hiding this comment.
I might be wrong but this would be a case that would fall under the domain layer category.
UserDeletionRepositoryFirestore is something that uses multiple repositories, and it is in-between the data layer, e.g. the repositories, and the UI layer, e.g. the view models.
This is exactly what the domain layer is defined as.
I suggest you (and @Redd87, have a look at it also) read about this article on the domain layer.
It describes what we do here as something that would fall in the domain layer, and that would be called a UseCase instead of a Repository.
It might seem strange because in Kotlin, calling use cases requires defining an invoke() function. I am not sure if this is 100% required but this seems to be what should be done, so it looks like a general convention. However I am not yet sure about how it would be defined in our case.
Anyways have a look at the domain layer explanation and give me your opinions. I think it would indeed fall under this category, so if we respect it then the MVVM architecture is fully respected (and we draw something under the domain layer in our architecture diagram!). If you agree, then I think the repository that was defined for the follow button would also fall in the domain category, e.g. ConcurrentAssociationUserRepositoryFirestore.
Concern:
However in both these cases, I am not sure how the runBatch would fall under this category, and how it could be replaced / done differently. Because these repos not only use multiple other repos, but also interact with the database directly, so I am not so sure about it being in the domain layer if we look at it from this perspective.
Either way, I think this might be a question worth asking the coaches.
Conclusion:
I think the current solution works well, and from the research I've done, these types of operations seem vague for everyone, meaning I haven't found a specific solution. From the looks of it, it seems up for debate. So I don't think it's really necessary to get into this right now, as the milestone is approaching, but we could definitely discuss it quickly.
|
I read the article and see what you mean. But doesn'r this just mean concretely that I can rename the file/class to "use case" instead of "Repository" ? In term of functionnality, I do not see any change to be done (The "invoke" operator you mentionned seems to not be mandatory to a "use case") |
Pretty much, yeah. Anyway it would make the purpose of the class much more clear, as it fits in between the repos and the view models. And in our case, the Let's wait for @Redd87's opinion, but if it's fine for him as well, this should be added in the domain layer of the architecture diagram, just like the concurrent association user repo made for the |
|
Ok, I'll make that change. Thank you very much for your comments, it was very helpful. |
|
@Zafouche I agree with your suggestions, it makes more sense this way. I also wonder if the searchRepository qualifies as such? |
I wouldn't say so. The guide I mentioned says that a |
There was a problem hiding this comment.
Good job on this! 🚀
Question: Is it hard to write tests for a batch operation?
I assume it is so I understand if they aren't added right now.
Also, just make sure to rename the repo to a UseCase and you are good to go.
Last thing, looks like the merge was rebased. If you do the merged through the terminal, I suggest you check you merging config. In this case, please just check it hasn't crushed anything that was implemented in the picture maximization feature PR, and that it is up to date with it (simple merge with main).
I honestly forgot a little how a rebase works, so be careful with that.
There was a problem hiding this comment.
Depending on what you choose for the interface, either rename it to UserDeletionUseCase, or UserDeletionUseCaseFirestore. Again, I'd go for UserDeletionUseCaseFirestore.
There was a problem hiding this comment.
I would either delete this interface, or rename it to UserDeletionUseCase. From what I see on the guide, a UseCase is directly implemented but it could be good to keep an interface for it, so I'd say to the latter.
| @Test | ||
| fun testFullSizePictureOnClick() { | ||
| setEventScreen(events[0]) |
There was a problem hiding this comment.
Good test addition, even though I think this is a test for a feature implemented in a different PR
| private fun <T> updateOrAdd(list: MutableList<T>, item: T, operation: (T) -> T) { | ||
| if (list.contains(item)) { | ||
| val index = list.indexOf(item) | ||
| val current = list[index] | ||
| val newItem = operation(current) | ||
| list[index] = newItem | ||
| } else { | ||
| val newItem = operation(item) | ||
| list.add(newItem) | ||
| } | ||
| } |
There was a problem hiding this comment.
Great addition. Add some small documentation to it though, event if it's straightforward to understand what it does.
| if (showFullScreen) { | ||
| PictureOverlay( | ||
| onDismiss = { | ||
| selectedPictureUri = Uri.EMPTY | ||
| showFullScreen = false | ||
| }, | ||
| pagerState, | ||
| eventPictures) | ||
| } |
There was a problem hiding this comment.
Is this supposed to be from the click on image feature addition? Why does it show up like an addition? Accidental rebase again? 🤣 Anyway just make sure it's on par with the main branch, so that it doesn't crush anything.
There was a problem hiding this comment.
Yep seems like you indeed merge rebased main
I do not know what cause this issue. I just switched to main to create another branch and working on it before switching back to it, I don't know which step could cause this. I have to check the merge configuration Update : I think I found where this come from. It's because I did a pull from main and because no conflict were found, git just did a "fast-forward" and register all these commit in my branch. So I think it is safe to merge this to main even with these commits that are not mine. I will be more careful in the futur to avoid "fast-forward" in PR like this. |
|
Ahh I see, fair enough. Have you tried doing a last updated from main to check you pulled all the commits? Edit: I just checked what commits were done by @armouldr on that picture maximization PR, and all of them are here, so no problem! Safe to merge. |
This issue was flagged here. It is part of polishing the inconsistencies of our app. Precisely, this means :