[16.0] Update fetchmail_server_folder #3222

ikus060 · 2025-03-06T20:24:08Z

Update fields description for clarity
Support fetch_last_day_only to fetch only 24 hours of emails
Process emails in reverse order
Use BODY.PEEK to avoid marking email as seen
Support email field 'reply_to' to matching
Improve logging
Relayout view to group related fields

OCA-git-bot · 2025-03-06T20:24:13Z

Hi @NL66278,
some modules you are maintaining are being modified, check this out!

NL66278 · 2025-03-07T09:09:10Z

fetchmail_attach_from_folder/models/fetchmail_server_folder.py

    )
-    active = fields.Boolean(default=True)
+    active = fields.Boolean(
+        string="Active",


Odoo will provide this capitalized name automatically, therefore should not be specified according to OCA guidelines.

Thanks I did not know

NL66278 · 2025-03-07T09:13:14Z

fetchmail_attach_from_folder/models/fetchmail_server_folder.py


    def get_criteria(self):
-        return "UNDELETED" if not self.fetch_unseen_only else "UNSEEN UNDELETED"
+        criteria = "UNDELETED"


Why replace one clear line, with three in the end doing the same?

If you want to improve it, take away the not operator:
return "UNSEEN UNDELETED" if self.fetch_unseen_only else "UNDELETED"

or, as you might still add the 24 hour crtierium:
criteria = "UNSEEN UNDELETED" if self.fetch_unseen_only else "UNDELETED"

one clear line

Let’s say clarity is subjective and differs from one individual to another.

Will make the changes

NL66278

Thank you for your contribution! To proceed a few things are needed. The first one is to locally use pre-commit before you push your changes. Use a pip virtual environment and pip install pre-commit. After activation of the virtual environment run pre-commit install in your repo directory. This will make your changes pass the pre-commit tests what is really required for a merge.

Another thing is to change strings on fields and help sparingly, as this will necessitate also new translations. Having said that your texts are in many cases a real improvement.

If you could add some unit test for the new functions, that would be nice, preventing a decrease in coverage.

ikus060 · 2025-03-07T13:04:36Z

I've make the requested changes in addition to running pre-commit hook.

NL66278 · 2025-03-07T13:19:02Z

@ikus060 Thank you for that. However pre-commit job is still complaining. Coud you run pre-commit run --all-files --show-diff-on-failure --color=always.

And if possible squash your changes into a single commit.

NL66278 · 2025-03-07T14:36:31Z

fetchmail_attach_from_folder/views/fetchmail_server.xml

+                                        <field
+                                            name="model_order"
+                                            placeholder="name asc"
+                                            attrs="{'readonly': [('match_first','==',False)],                                                     'required': [('match_first','==',True)]}"


Not sure what happened here, but the string starting with 'required'could go on the next line, under attrs (or under readonly).

NL66278 · 2025-03-07T14:37:52Z

fetchmail_attach_from_folder/views/fetchmail_server.xml

+                                <group>
+                                    <separator string="Matching Options" />
+                                    <group colspan="2">
+                                      <field


The field element should be indented 4 spaces under group, not sure why pre-commit prettify did not pick this up.

NL66278 · 2025-03-07T14:41:09Z

@ikus060 Just some small remarks on the xml formatting and we should be good to go...

NL66278 · 2025-03-07T14:45:06Z

@ikus060 You might also have a look at my PR to use message uid instead of number in folder. I think the same problem you solve with handling the messages in reverse order. #3017

ikus060 · 2025-03-07T16:50:25Z

Here another tentative. Sorry for all the noise. I'm not used to OCA code policies.

See my comments for #3017

NL66278 · 2025-03-07T16:59:59Z

@ikus060 No problem. They are here: https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst

NL66278

👍 LGTM

ikus060 · 2025-03-11T11:17:57Z

Is there something blocking the merge ?

NL66278 · 2025-03-11T11:21:58Z

@ikus060 Nothing really blocking, but waiting for more reviews. @hbrunn Maybe you want to have a look?

zamberjo

Thank you for your pr, I would like to point out a few points I have observed.

zamberjo · 2025-03-14T06:21:53Z

fetchmail_attach_from_folder/models/fetchmail_server_folder.py

-        return "UNDELETED" if not self.fetch_unseen_only else "UNSEEN UNDELETED"
+        criteria = "UNSEEN UNDELETED" if self.fetch_unseen_only else "UNDELETED"
+        if self.fetch_last_day_only:
+            yesterday = (datetime.utcnow() - timedelta(days=1)).strftime("%d-%b-%Y")


I think it is preferable to use the tools already included in Odoo.

Suggested change

yesterday = (datetime.utcnow() - timedelta(days=1)).strftime("%d-%b-%Y")

yesterday = fields.Date.subtract(fields.Date.context_today(self), days=1).strftime("%d-%b-%Y")

zamberjo · 2025-03-14T06:22:28Z

fetchmail_attach_from_folder/models/fetchmail_server_folder.py

+        criteria = "UNSEEN UNDELETED" if self.fetch_unseen_only else "UNDELETED"
+        if self.fetch_last_day_only:
+            yesterday = (datetime.utcnow() - timedelta(days=1)).strftime("%d-%b-%Y")
+            criteria = f"SINCE {yesterday} " + criteria


A little more legible

Suggested change

criteria = f"SINCE {yesterday} " + criteria

criteria = f"SINCE {yesterday} {criteria}"

zamberjo · 2025-03-14T06:26:22Z

fetchmail_attach_from_folder/models/fetchmail_server_folder.py

        """Select a single message from a folder."""
        self.ensure_one()
-        result, msgdata = connection.fetch(msgid, "(RFC822)")
+        result, msgdata = connection.fetch(msgid, "(BODY.PEEK[])")


Do you think this can be configurable?

Hum, good point. I would recommand to do something similar to flag_nonmatching.

I could introduce a new fields to mark the email as seen if matching. and another fields to mark the email as seed if not matching ?

The idea is to let the admin decide which email should be mark as seen.

Maybe you have a different proposal ?

To make it symetric with flag_nonmatching, I could also add a field flag_matching.

@ikus060 @zamberjo Would definitely be nice to have this configurable! I do not quite see what the purpose would be of a field flag_matching. The idea of flagging is to make clear to the user that some kind of manual action might be needed, as we do not know what to do with the email in Odoo. As for configuring whether to use BODY.PEEK or RFC822 I would prefer a selection field. (And then preferably have RFC822 as the default, as that is the current way it works and on which organizations might rely). The idea would by default be that users only occasionally look at the mailbox, and then can assume UNSEEN mails have not been fetched by Odoo. On the other hand, if in some organizations somebody must check the mailbox regularly and actually read the emails, BODY.PEEK would be appropriate to make clear to the user which mails this user still needs to deal with.

I agree with @NL66278, imho the best thing to do is to create a selection field leaving RFC822 as default.

Since the only difference between BODY.PEEK and RFC822 is that the latter marks the e-mail as read, it would be preferable to offer a more flexible configuration to support other usage scenarios, including :

Marking all e-mails as read (default): This is the previous behaviour of this addons. Any email consume by the addons are marked as Seen.

Mark matching e-mails as read : When email is not matchhing anything, then the email is left Unseed.

Do not mark e-mails as read : This is the behaviour I'm looking for. My use case is to have Odoo consume all email from inbox and attach matching email to the right ressources for tracking and archiving. Users continue to use their mailbox as usual.

If you approve this direction. Do you still recommand a selection field ?

@ikus060 Having three options I think actually strengthens the case for a selection field.

- Update fields description for clarity - Support fetch_last_day_only to fetch only 24 hours of emails - Process emails in reverse order - Use BODY.PEEK to avoid marking email as seen - Support email field 'reply_to' to matching - Improve logging - Relayout view to group related fields - Add `seen` selection field Add `seen` selection field

ikus060 · 2025-04-03T18:04:48Z

Hello All,

I took some time to update this Pull Request with the changes recommended.

NL66278 · 2025-04-04T12:41:56Z

@zamberjo I think this is ready for merge now. Can you also have another look?

zamberjo

LGTM

Maybe in the future someone will be interested in configuring that period of time instead of leaving it fixed at 24 hours, but let's leave it like that for the moment.

NL66278 · 2025-04-14T16:27:50Z

/ocabot merge minor

OCA-git-bot · 2025-04-14T16:28:01Z

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-3222-by-NL66278-bump-minor, awaiting test results.

Signed-off-by NL66278

OCA-git-bot · 2025-04-14T16:35:32Z

@NL66278 your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-3222-by-NL66278-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

NL66278 · 2025-04-15T07:13:53Z

/ocabot merge minor

OCA-git-bot · 2025-04-15T07:14:03Z

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-3222-by-NL66278-bump-minor, awaiting test results.

Signed-off-by NL66278

OCA-git-bot · 2025-04-15T07:21:34Z

@NL66278 your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-3222-by-NL66278-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

NL66278 · 2025-04-15T07:28:19Z

For some reason merge commits fails on tests that ran successfully when submitting the PR. On modules that have nothing to do with this PR.

hbrunn · 2025-04-25T09:34:47Z

@NL66278 that's normal behavior, tests are run on the head branch with the PR merged in. And this fails currently because of changes in Odoo, fixed in #3271

@ikus060 next time you create a PR please give it a name that's more descriptive than "Update ..."

NL66278 · 2025-05-01T11:50:48Z

Conflicts can be resolved by merging this PR: https://github.com/ikus060/server-tools/pull/1

thomaspaulb · 2026-01-25T21:56:33Z

@ikus060 would like to merge but can you rebase and solve conflicts, with help of @NL66278 pr

[FIX] fetchmail_attach_from_folder: solve conflicts

ikus060 · 2026-01-26T12:35:33Z

@thomaspaulb It should be merged.

thomaspaulb · 2026-01-26T12:38:16Z

@ikus060 There are still conflicts:

Also, the commits should be squashed after the conflicts are solved.

OCA-git-bot · 2026-01-26T12:41:07Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

ikus060 · 2026-01-26T12:58:10Z

@thomaspaulb Thanks for getting back to me on this PR.

I appreciate that we're moving forward with the merge. But, I want to be transparent about where I'm at: it's been almost a year since I submitted this, and in that time my focus has shifted to other projects. I also no longer have the development environment set up to properly test these changes.

I completely understand that everyone involved is volunteering their time, and I'm grateful for that. At the same time, I think this situation highlights how long review cycles can make it challenging for contributors to stay engaged - especially when we need to context-switch back after many months.

I now resolve the conflict using Github tools. Let hope it's working as expected.

If not, I will need more time to recreate all the setup environment.

thomaspaulb · 2026-01-26T13:10:38Z

@ikus060 I understand where you're coming from. OCA currently has this problem of long review cycles. We're trying at the Board level to see how to fix this, but it's not easy.

On the other hand, you don't need a development environment to fix this. You just check out the branch on Git, run some smart git commands, and re-push. If not, this will go stale and some other developer will have to open a new PR and fix it for you.

ikus060 · 2026-01-26T15:44:26Z

@thomaspaulb I believe I did everything needed. I don't see any conflict anymore.

thomaspaulb · 2026-01-26T15:46:15Z

I still see it, but maybe it's my Github.

ikus060 · 2026-01-26T16:11:44Z

@thomaspaulb if I understand it well, this to check verify the unit test code coverage. Am I right ?

It seems I need to write new units tests. Am I really required to write those test ? I would prefer not to, as it's not a priority for me.

thomaspaulb · 2026-01-26T21:44:52Z

@ikus060 Where do you get the idea that you have to write new unit tests in order to solve a Git merge conflict?

The only thing that's needed, basically, is something like

git clone git@github.com:OCA/server-tools@patrik-fetchmail-server-folder server-tools
cd server-tools
git fetch origin 16.0
git remote add ikus git@github.com:ikus060/server-tools
git fetch ikus patrik-fetchmail-server-folder
git checkout -b  patrik-fetchmail-server-folder ikus/patrik-fetchmail-server-folder
git rebase -i origin/16.0
# leave the commits the should be there
# solve merge conflicts as they come
git add .
pre-commit run  # sanity check
# then when happy squash commits into one
git push -f

It should be, at max, a 15 min job, if not even that, and not worth the discussion IMO

thomaspaulb · 2026-01-26T21:46:31Z

Ah, I think you looked at the wrong part of the screenshot. It is not required to make the coverage green. The important thing to fix is the merge conflicts, which are visible at the bottom of the screenshot as This branch cannot be rebased due to conflicts.

NL66278 · 2026-01-26T22:00:47Z

@ikus060 @thomaspaulb There will be more conflicts after merging my backports of Odoo 18.0 improvements. I made a branch with the improvements offered by @ikus060 based on the backport branch, so I propose first looking at the backport branch and if OK merging, then merging in the improvement branch, which basically is already approved in the reviews here.

The branch containing both backport and improvements: https://github.com/Therp/server-tools/tree/patrik-fetchmail-server-folder

NL66278 · 2026-01-27T11:13:29Z

@ikus060 Would you have a quick look / review of this branch? If I merge it in, then I can proceed to merge your changes.

ikus060 · 2026-01-30T19:45:41Z

Closing this PR. Reopen for 18.0 branch #3510

NL66278 reviewed Mar 7, 2025

View reviewed changes

NL66278 suggested changes Mar 7, 2025

View reviewed changes

ikus060 force-pushed the patrik-fetchmail-server-folder branch from 70ca8df to bd2ad4a Compare March 7, 2025 13:23

NL66278 reviewed Mar 7, 2025

View reviewed changes

ikus060 force-pushed the patrik-fetchmail-server-folder branch from bd2ad4a to f00334b Compare March 7, 2025 16:46

NL66278 approved these changes Mar 7, 2025

View reviewed changes

ikus060 mentioned this pull request Mar 7, 2025

[16.0][FIX] fetch...folder: use message uid's not sequence #3017

Merged

zamberjo reviewed Mar 14, 2025

View reviewed changes

rvalyi changed the title ~~Update fetchmail_server_folder~~ [16.0] Update fetchmail_server_folder Mar 18, 2025

ikus060 force-pushed the patrik-fetchmail-server-folder branch from f00334b to 0791fdc Compare April 3, 2025 18:03

zamberjo approved these changes Apr 7, 2025

View reviewed changes

OCA-git-bot added the approved label Apr 7, 2025

OCA-git-bot added a commit that referenced this pull request Apr 14, 2025

Merge PR #3222 into 16.0

a6062b2

Signed-off-by NL66278

OCA-git-bot added the bot is merging ⏳ label Apr 14, 2025

OCA-git-bot removed the bot is merging ⏳ label Apr 14, 2025

OCA-git-bot added a commit that referenced this pull request Apr 15, 2025

Merge PR #3222 into 16.0

36f4705

Signed-off-by NL66278

OCA-git-bot added the bot is merging ⏳ label Apr 15, 2025

OCA-git-bot removed the bot is merging ⏳ label Apr 15, 2025

[FIX] fetchmail_attach_from_folder: solve conflicts

85ee260

Merge pull request #1 from Therp/patrik-fetchmail-server-folder

e5e4e5f

[FIX] fetchmail_attach_from_folder: solve conflicts

OCA-git-bot added the ready to merge label Jan 26, 2026

Merge branch '16.0' into patrik-fetchmail-server-folder

27e65f9

ikus060 closed this by deleting the head repository Jan 30, 2026

	yesterday = (datetime.utcnow() - timedelta(days=1)).strftime("%d-%b-%Y")
	yesterday = fields.Date.subtract(fields.Date.context_today(self), days=1).strftime("%d-%b-%Y")

	criteria = f"SINCE {yesterday} " + criteria
	criteria = f"SINCE {yesterday} {criteria}"

Uh oh!

[16.0] Update fetchmail_server_folder #3222

[16.0] Update fetchmail_server_folder #3222

Conversation

ikus060 commented Mar 6, 2025

Uh oh!

OCA-git-bot commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NL66278 Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NL66278 left a comment

Choose a reason for hiding this comment

Uh oh!

ikus060 commented Mar 7, 2025

Uh oh!

NL66278 commented Mar 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NL66278 commented Mar 7, 2025

Uh oh!

NL66278 commented Mar 7, 2025

Uh oh!

ikus060 commented Mar 7, 2025

Uh oh!

NL66278 commented Mar 7, 2025

Uh oh!

NL66278 left a comment

Choose a reason for hiding this comment

Uh oh!

ikus060 commented Mar 11, 2025

Uh oh!

NL66278 commented Mar 11, 2025

Uh oh!

zamberjo left a comment

Choose a reason for hiding this comment

Uh oh!

zamberjo Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikus060 commented Apr 3, 2025

Uh oh!

NL66278 commented Apr 4, 2025

Uh oh!

zamberjo left a comment

Choose a reason for hiding this comment

Uh oh!

NL66278 commented Apr 14, 2025

Uh oh!

OCA-git-bot commented Apr 14, 2025

Uh oh!

OCA-git-bot commented Apr 14, 2025

Uh oh!

NL66278 commented Apr 15, 2025

Uh oh!

NL66278 Mar 7, 2025 •

edited

Loading

zamberjo Mar 14, 2025 •

edited

Loading