Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Snooze mvp follow up #8754

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Snooze mvp follow up #8754

merged 1 commit into from
Aug 17, 2023

Conversation

JohannesGGE
Copy link
Contributor

@JohannesGGE JohannesGGE commented Aug 15, 2023 •
edited
Loading

Fix: #8747

This follow up is fixing some minor things:

  • Update snooze options aligned to fix(MessageButtonsBar) - adjust predefined reminders conditions spreed#10177
  • Set alarm icon for selected dedicated mailbox (not just name match "Snoozed")
  • Only one "snooze" request to the backend (not snooze and move)
  • Enable snoozing message if DB entry for the message already exists
  • Remove DB entry for the message if moving fails
  • Move snooze logic to SnoozeService

@JohannesGGE JohannesGGE self-assigned this Aug 15, 2023
@JohannesGGE JohannesGGE force-pushed the fix/snooze-mvp-follow-up branch 2 times, most recently from bde3b91 to c39cc58 Compare August 16, 2023 15:28
@JohannesGGE JohannesGGE marked this pull request as ready for review August 16, 2023 15:30
ChristophWurst
ChristophWurst reviewed Aug 16, 2023
View reviewed changes
lib/Service/SnoozeService.php Outdated
);
} catch (\Throwable $e) {
$this->unSnoozeMessageDB($message);
throw new ServiceException($e->getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

loses all of the previous error trace. either just rethrow the original throwable or pass the original as previous of the new service exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅

lib/Service/SnoozeService.php Outdated
);
} catch (\Throwable $e) {
$this->unSnoozeThreadDB($selectedMessage);
throw new ServiceException($e->getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅

lib/Service/SnoozeService.php
$this->messageSnoozeMapper->insert($snooze);
try {
$this->messageSnoozeMapper->insert($snooze);
} catch(Exception $e) {
Copy link
Member

Choose a reason for hiding this comment

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

can you check the reason of the error and only handle duplicates and rethrow the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check $e->getPrevious() instanceof \Doctrine\DBAL\Exception\UniqueConstraintViolationException. Do you think this is okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, tests fail because the UniqueConstraintViolationException is part of 3rdparty. Is there a way to use it without errors or is this not an option?

Copy link
Member

Choose a reason for hiding this comment

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

\OCP\DB\Exception::getReason and comparison with \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION

lib/Service/SnoozeService.php Outdated
Comment on lines 221 to 257
$messages = $this->threadMapper->findMessageIdsByThreadRoot(
$selectedMessage->getThreadRootId()
);

foreach ($messages as $message) {
$this->messageSnoozeMapper->deleteByMessageId($message['messageId']);
}
Copy link
Member

Choose a reason for hiding this comment

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

perf: would be faster and atomic if this is done in one query by the db

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed deleteByMessageId() to take array of ids and query db by where->in

@JohannesGGE JohannesGGE force-pushed the fix/snooze-mvp-follow-up branch 5 times, most recently from 481fb2d to bb369ef Compare August 17, 2023 15:02
ChristophWurst
ChristophWurst approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good!

lib/Service/SnoozeService.php Outdated
@@ -144,6 +285,7 @@ private function wakeMessagesByAccount(Account $account): void {

$client = $this->clientFactory->getClient($account);
try {
$messageIdsToDelete = array();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$messageIdsToDelete = array();
$messageIdsToDelete = [];

@JohannesGGE
fix(Snooze): some minor changes
326d48f 
Signed-off-by: Johannes Merkel <[email protected]>
@JohannesGGE JohannesGGE merged commit 5969aa8 into main Aug 17, 2023
28 of 29 checks passed
@JohannesGGE JohannesGGE deleted the fix/snooze-mvp-follow-up branch August 17, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@st3iny st3iny Awaiting requested review from st3iny

@kesselb kesselb Awaiting requested review from kesselb kesselb is a code owner

Assignees

@JohannesGGE JohannesGGE

Labels
3. to review bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Snooze MVP follow up
2 participants
@JohannesGGE @ChristophWurst