feat: add and remove conversation favorite [WPB-11639] #3119
Conversation
Garzas
requested review from
yamilmedina,
MohamadJaara,
vitorhugods,
saleniuk and
damian-kaczmarek
echoes-hq
bot
added
the
echoes: product-roadmap
Garzas
changed the title
|
Bencher Report| Branch | feat/manage-favorite-folder |
| Testbed | ubuntu-latest |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsCLI flag.
Click to view all benchmark results
| Benchmark | Latency | nanoseconds (ns) |
|---|---|---|
| com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInFiles | 📈 view plot | 652,050.28 |
| com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInMemory | 📈 view plot | 378,035,743.70 |
| com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.messageInsertionBenchmark | 📈 view plot | 1,313,351,898.18 |
| com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.queryMessagesBenchmark | 📈 view plot | 21,444,470.42 |
Datadog ReportBranch report: ✅ 0 Failed, 3216 Passed, 107 Skipped, 40.56s Total Time |
|
| END AS interactionEnabled, | ||
| CASE WHEN EXISTS ( | ||
| SELECT 1 | ||
| FROM LabeledConversation lc | ||
| JOIN ConversationFolder cf ON lc.folder_id = cf.id | ||
| WHERE lc.conversation_id = Conversation.qualified_id | ||
| AND cf.folder_type = 'FAVORITE' | ||
| ) | ||
| THEN 1 ELSE 0 END AS isFavorite |
There was a problem hiding this comment.
suggestion: making multiple nested queries can be expensive.
I would suggest to change it to use joins, like:
LabeledConversation.folder_id IS NOT NULL AS isFavorite
FROM Conversation
LEFT JOIN ConversationFolder AS FavoriteFolder ON FavoriteFolder.folder_type = 'FAVORITE'
LEFT JOIN LabeledConversation ON LabeledConversation.conversation_id = Conversation.id AND LabeledConversation.folder_id = FavoriteFolder.id
First left join filters out and joins only favorite folder, second left join adds given conversation's folders but filters them so that it only adds the one which matches with favorite folder id, so in the end LabeledConversation.folder_id is equal to favorite folder id or NULL if this conversation is not in favorite folder 😄
| @@ -17,6 +17,19 @@ CREATE TABLE LabeledConversation ( | |||
| PRIMARY KEY (folder_id, conversation_id) | |||
| ); | |||
|
|
|||
| getFolderWithConversations: | |||
There was a problem hiding this comment.
question: is it supposed to return all folders? Then maybe name it getAllFoldersWithConversations? 🤔
| @@ -0,0 +1,125 @@ | |||
| /* | |||
There was a problem hiding this comment.
change: name of the file is RemoveConversationFromFaroritesUseCase without Test at the end 😄
| return wrapStorageRequest { | ||
| conversationFolderDAO.addConversationToFolder(conversationId.toDao(), folderId) | ||
| } | ||
| .flatMap { | ||
| syncConversationFoldersFromLocal() | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd suggest to move syncConversationFoldersFromLocal() to the usecase.
Repository should stay as dummy class, it just interacts with data sources by providing CRUD operations and do data transformation.
| }.flatMap { | ||
| syncConversationFoldersFromLocal() | ||
| } |
| override suspend fun syncConversationFoldersFromLocal(): Either<CoreFailure, Unit> { | ||
| kaliumLogger.withFeatureId(CONVERSATIONS_FOLDERS).v("Syncing conversation folders from local") | ||
| return wrapStorageRequest { conversationFolderDAO.getFoldersWithConversations().map { it.toModel() } } | ||
| .flatMap { | ||
| wrapApiRequest { | ||
| userPropertiesApi.updateLabels( | ||
| LabelListResponseDTO(it.map { it.toLabel() }) | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is also breaking the separation of concerns.
We can have this logic in a use case, as it's responsible for orchestrating business logic.
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
syncConversationFoldersFromLocalfor updating all folders from local to APIisFavoriteto conversation details view