Skip to content

[#5783] Improve resilience to unparsable outlook attachments#5920

Merged
gbp merged 3 commits intodevelopfrom
5783-encoding-compatibility-error-resilience
Oct 15, 2020
Merged

[#5783] Improve resilience to unparsable outlook attachments#5920
gbp merged 3 commits intodevelopfrom
5783-encoding-compatibility-error-resilience

Conversation

@garethrees
Copy link
Copy Markdown
Member

@garethrees garethrees commented Oct 13, 2020

Sometimes we receive messages with outlook attachments that can't be
parsed due to an issue in mapi [1].

There's a potential fix [2] but it conflicts [3] with an existing patch
we apply [4].

This at least allows users to download the raw attachment, rather than
us preventing the entire request from loading because we raise an
exception.

Part of #5783.

[1] aquasync/ruby-msg#15
[2] aquasync/ruby-msg#16
[3] #5783 (comment)
[4] mysociety/ruby-msg#3

Notes to reviewer

Do not introduce global variables.

We actually rely on $alaveteli_dir in this case, as we use it in MailHandler::ReplyHandler.load_rails.

@garethrees garethrees changed the title Improve resilience to unparsable outlook attachments [#5783] Improve resilience to unparsable outlook attachments Oct 13, 2020
We try to minimise what we load in `script/handle-mail-replies` as it's
run pretty frequently, so we want to avoid memory capacity problems.

This commit does some minor cleanup to group the addition of a directory
to the load path, the requires for the files under that load path, and
any code called directly from the require.

This is to make a future change easier. I've also cleaned up quote
style.
Sometimes we receive messages with outlook attachments that can't be
parsed due to an issue in mapi [1].

There's a potential fix [2] but it conflicts [3] with an existing patch
we apply [4].

This at least allows users to download the raw attachment, rather than
us preventing the entire request from loading because we raise an
exception.

It's not easy to include an attachment in the specs to replicate a real
error case due to the complexity of removing PII, so I've just stubbed
the call to `.open` as we don't care about the specifics in this spec.

`script/handle-mail-replies` needs an explicit require as we minimise
the load path for that script.

Part of #5783.

[1] aquasync/ruby-msg#15
[2] aquasync/ruby-msg#16
[3] #5783 (comment)
[4] mysociety/ruby-msg#3
@mysociety-pusher mysociety-pusher force-pushed the 5783-encoding-compatibility-error-resilience branch from 2c6597b to edd9c9c Compare October 14, 2020 12:05
Fixes RuboCop warning.
@garethrees garethrees marked this pull request as ready for review October 14, 2020 13:18
@gbp gbp self-requested a review October 15, 2020 08:22
@gbp gbp self-assigned this Oct 15, 2020
Copy link
Copy Markdown
Member

@gbp gbp 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 to me 👍

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.

2 participants