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.

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

Add tagging to messages #4665

Merged
merged 1 commit into from
Mar 22, 2021
Merged

Conversation

miaulalala
Copy link
Contributor

As discussed in #25

lib/Db/Tag.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Migration/Version1100Date20210304143008.php Outdated Show resolved Hide resolved
src/store/actions.js Outdated Show resolved Hide resolved
@miaulalala miaulalala force-pushed the enhancement/tagging-messages branch 3 times, most recently from aec19d5 to 63ff4c3 Compare March 11, 2021 14:08
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
@ChristophWurst

This comment has been minimized.

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.

Some little quirks I found. Otherwise reading the tags from Thunderbird into our DB works as expected :)

I noticed this as well:

Bildschirmfoto von 2021-03-16 09-51-09

(accidentally added the same account multiple times)

Is that a problem? Checking before insert is likely expensive, so we might accept the cost of duplication.

lib/Service/SetupService.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@miaulalala

This comment has been minimized.

@ChristophWurst
Copy link
Member

CI is partly failing because of codecov/codecov-action#234

lib/Db/Message.php Show resolved Hide resolved
lib/Db/Message.php Outdated Show resolved Hide resolved
lib/Db/Message.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/MessageMapper.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Show resolved Hide resolved
lib/Service/MailManager.php Show resolved Hide resolved
lib/Service/Search/Flag.php Outdated Show resolved Hide resolved
lib/Service/SetupService.php Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Member

ChristophWurst commented Mar 18, 2021

  • BUG message tags are always on the front-end

in \OCA\Mail\Db\Message::jsonSerialize you use \OCA\Mail\Db\Message::getTags but the tags prop is only set when when read messages from imap.

We need something similar to \OCA\Mail\Db\MessageMapper::findRecipients in the message mapper that loads each message's tags.

@ChristophWurst
Copy link
Member

miaulalala force-pushed the enhancement/tagging-messages branch

unless there is a change on master that we need here I would recommend to only do fixups and wait with the rebase until we have positive review & CI result. That will make it easier for me and others to check out the branch and pull the latest changes.

@ChristophWurst
Copy link
Member

ChristophWurst commented Mar 18, 2021

  • BUG our old $important tag in inserted as a new tag, but we should just map it to $label1

Bildschirmfoto von 2021-03-18 10-30-17

@miaulalala miaulalala force-pushed the enhancement/tagging-messages branch 5 times, most recently from dab7583 to bf2bc3d Compare March 22, 2021 09:56
@ChristophWurst
Copy link
Member

ChristophWurst commented Mar 22, 2021

  • BUG I could have happened because of the way I reset my db before I re-migrated to your new schema changes here but this shouldn't happen:

Bildschirmfoto von 2021-03-22 12-10-50

@ChristophWurst
Copy link
Member

ChristophWurst commented Mar 22, 2021

  • BUG message.tags is not an indexed object, but used as one

Bildschirmfoto von 2021-03-22 12-13-07

we have code that does envelope.tags.$label1 but this only works it envelope.tags is an object with a key $label1. when we load messages/envelopes they have an array of tags, however. So we have to use something like a envelope.tags.some(t => t.keyword === '$label1') with a O(n) or we store the tags as a simpler object of keywords, like { '$label1': { id: 123, keyword: '$label1', displayName: 'Important' } and then we can (again) use envelope.tags.$label1 with O(1).

To sum up we should index tags by their keyword.

lib/Db/Tag.php Outdated Show resolved Hide resolved
lib/Db/Tag.php Outdated Show resolved Hide resolved
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.

Proper types and their documentation help developers, IDEs and static analysis :)

lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Db/TagMapper.php Outdated Show resolved Hide resolved
lib/Migration/Version1100Date20210304143008.php Outdated Show resolved Hide resolved
lib/Model/IMAPMessage.php Outdated Show resolved Hide resolved
lib/Model/IMAPMessage.php Outdated Show resolved Hide resolved
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.

Tested and works :shipit:

Now you may do the final rebase & squash done

@ChristophWurst ChristophWurst mentioned this pull request Mar 22, 2021
8 tasks
Signed-off-by: Anna Larch <anna@nextcloud.com>
@ChristophWurst ChristophWurst merged commit 99381f7 into master Mar 22, 2021
@ChristophWurst ChristophWurst deleted the enhancement/tagging-messages branch March 22, 2021 18:09
This was referenced Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants