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

Implement 'flag' action #61

Merged
merged 3 commits into from
Dec 21, 2020

Conversation

SpotlightKid
Copy link
Contributor

@SpotlightKid SpotlightKid commented Dec 7, 2020

Implements a new flag action, with which flags can be set or added and/or removed on messages.

The action expects either a "set" or an "add" and / or a "remove" entry, which specify the flag(s) to set or add and / or remove. The entry value can either be a single flag or a list of flags. An IMAP message flag is a string that usually starts with a backslash, e.g. \Seen. You can use one of the following constants for commonly used flags:

ANSWERED = r'\Answered'
DELETED = r'\Deleted'
DRAFT = r'\Draft'
FLAGGED = r'\Flagged'
SEEN = r'\Seen'

Examples:

action:
  name: flag
  set:
  - FLAGGED
  - SEEN

This would set the \Flagged and \Seen flags on the message and remove all other flags.

And:

action:
    name: flag
    add: FLAGGED
    remove: SEEN

This would add the \Flagged flag and remove the \Seen flag from the message but leave all other existing flags in place.

@SpotlightKid
Copy link
Contributor Author

Documentation and tests to follow.

@SpotlightKid
Copy link
Contributor Author

Added documentation.

Copy link
Member

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

The implementation looks correct, but I have some comments about the config format. Let me know what you think.

action:
name: flag
add: FLAGGED
remove: SEEN
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about something a little more declarative. Maybe like this?

action:
  name: flag
  flagged: yes
  seen: no

Any flags that are not mentioned would not be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks nice but wouldn't allow to add any custom flags (aka keywords). I cite RFC-2060:

A system flag is a flag name that is pre-defined in this
specification. All system flags begin with "". Certain system
flags (\Deleted and \Seen) have special semantics described
elsewhere. The currently-defined system flags are:
[...]
A keyword is defined by the server implementation. Keywords do
not begin with "". Servers MAY permit the client to define new
keywords in the mailbox (see the description of the
PERMANENTFLAGS response code for more information). [...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could have separate actions for the system flags, e.g. read, a draft, etc. actions (like the delete action) and use the flag option only for custom keywords and non-standard system-flags, but I have to admit that I would find this unnecessarily complicated.

Or allow both the set resp. add / remove options and have additional boolean options for read, answered, flagged and draft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about this some more, your proposal would also make it impossible to specify a specific set of flags (i.e. what set does now), because you don't know, which flags are already present (assuming there may be other flags than the system flags).

Copy link
Member

Choose a reason for hiding this comment

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

OK, I'm not sure we're thinking about using this in the same way, but I think I get your approach. It seems you envisioned a more complete/complex set of use cases than I have in mind.

Re-reading #8 and #9, the request wasn't to the IMAP flags API directly to users, it was to provide a way to manage a couple of specific flags, using the terminology someone familiar with a mail client would expect. What I had in mind for #8 was to add/remove the "flagged" flag. That's why #9 is filed separately for managing read/unread status. I don't think we want the other options exposed at all.

The "answered" flag seems like something the mail client is going to set when you reply to a message, and I'm not sure I would want to change its value as part of sorting messages between mailboxes.

The same for "draft" -- I expect that's used while a message is being composed, and I wouldn't want it changed until I sent the message.

I guess I could see someone removing the "deleted" flag to undelete a message, but I doubt I would use that myself. Is that useful to you?

I've never heard of the ability to set custom flags. Do people do that? In what way? What do we do if the server doesn't support that feature (die with an error, I suppose)? If we did expose this, I could see it working as you've described set except that it would ignore all of the standard flags and only manage the custom flags provided by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, as I see it, #8 and #9 were FRs for specialized instances of setting IMAP message flags. Why implement something restricted, when it it's easier to implement the general case and it is, in my opinion, just as easy to use.

True, in most cases, people would only want to mark a message as important or set its status to read/unread. So maybe it just needs better examples in the documentation, how to do that.


For example, to mark a message as important or needing special attention, add the FLAGGED flag to it. Your mail client will then probably show the message marked with a star or similar:

mailboxes:
- name: INBOX
  rules:
  - headers:
    - name: "from"
      substring: "boss@mycompany.com"
    action:
      name: flag
      add: FLAGGED

As another example, to mark a message as read, just add the SEEN flag to it. You mail client will then display the message as though you already viewed it:

mailboxes:
- name: INBOX
  rules:
  - headers:
    - name: "subject"
      substring: "daily newsletter"
    action:
      name: flag
      add: SEEN

Conversely, to mark a message as unread, just remove the SEEN flag:

action:
  name: flag
  remove: SEEN

(Adding READ as an another alias for \Seen would be, of course, no problem.)

My maxim here is: common things should simple, but specialized needs should still be possible.

Copy link
Member

Choose a reason for hiding this comment

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

So, as I see it, #8 and #9 were FRs for specialized instances of setting IMAP message flags. Why implement something restricted, when it it's easier to implement the general case and it is, in my opinion, just as easy to use.

I don't agree about the usability.

True, in most cases, people would only want to mark a message as important or set its status to read/unread. So maybe it just needs better examples in the documentation, how to do that.

Those examples are better, but actions like "mark_read" and "mark_unread" are even clearer and simpler to understand.

My maxim here is: common things should simple, but specialized needs should still be possible.

I would prefer for users of imapautofiler to not have to understand the IMAP protocol to use the tool, any more than they have to understand it to use their regular mail client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it seems more discussion is necessary, how these commands should look. Let's do that in #8 resp. #9.

DELETED = r'\Deleted'
DRAFT = r'\Draft'
FLAGGED = r'\Flagged'
SEEN = r'\Seen'
Copy link
Member

Choose a reason for hiding this comment

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

"Seen" is the IMAP standard, but I'm not sure most users think of it that way. Most clients use "read" and "unread". Maybe we could make them aliases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since they are the inverse of each other, you need to add \Seen to mark a message as read, and remove \Seen to mark it as unread. A 1:1 alias would therefor not work with the current configuration scheme. And turning add: UNREAD into remove: SEEN seems prone to cause confusion, esp. in conjunction with other flags in the same action.

Signed-off-by: Christopher Arndt <chris@chrisarndt.de>
Signed-off-by: Christopher Arndt <chris@chrisarndt.de>
Signed-off-by: Christopher Arndt <chris@chrisarndt.de>
@SpotlightKid
Copy link
Contributor Author

Added unit tests.

@SpotlightKid SpotlightKid changed the title WIP: Implement 'flag' action Implement 'flag' action Dec 7, 2020
@SpotlightKid SpotlightKid marked this pull request as draft December 9, 2020 14:21
@dhellmann dhellmann mentioned this pull request Dec 21, 2020
@dhellmann
Copy link
Member

#62 shows what I had in mind for the flag/unflag actions.

@dhellmann dhellmann merged commit 0f24086 into imapautofiler:master Dec 21, 2020
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.

None yet

2 participants