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

Move mailboxes code to separate app #295

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

kevincarrogan
Copy link
Contributor

@kevincarrogan kevincarrogan commented Dec 22, 2024

At the moment nearly all of the lite-hmrc code resides in a single Django application called mail. This makes it quite overwhelming to understand where to look for code when debugging and diagnosing problems.

The main purpose of this PR is to split out one aspect of the mail application which is the code related to mail servers and mailboxes.

As part of this there are a few bonus extras as well:

  • Adding integration tests for these new apps
  • Allowing mail servers to be fully defined in config (this makes testing easier)
  • Removal of unused code (mock_hmrc and ICMS related code)
  • Removal of Django Choices code as we now have it available directly from Django
  • Simplified a lot of email parsing using the Python email standard library
  • Rework the pulling of messages from a mailbox to, hopefully, make it more understandable
  • Add a lot of tests to cover all of above changes

@kevincarrogan kevincarrogan force-pushed the LTD-5750-mailboxes branch 10 times, most recently from cb05ffb to 27bf2fc Compare December 24, 2024 16:35
@kevincarrogan kevincarrogan force-pushed the LTD-5749-mail-server-functionality branch from 55acaf8 to 4742957 Compare December 27, 2024 11:24
@kevincarrogan kevincarrogan changed the base branch from LTD-5749-mail-server-functionality to master December 27, 2024 11:25
@kevincarrogan kevincarrogan force-pushed the LTD-5750-mailboxes branch 6 times, most recently from e3dc962 to 575e057 Compare December 31, 2024 11:40
@kevincarrogan kevincarrogan marked this pull request as ready for review December 31, 2024 11:42
@kevincarrogan kevincarrogan changed the base branch from master to LTD-5755-integration-tests December 31, 2024 12:10
Base automatically changed from LTD-5755-integration-tests to master January 2, 2025 15:19
@currycoder currycoder self-requested a review January 6, 2025 16:04
mailboxes/utils.py Outdated Show resolved Hide resolved
logger = logging.getLogger(__name__)


def get_message_number(listing_message):
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like some docstrings on these utils would be a help for my understanding.

MailReadStatuses.UNPROCESSABLE,
]
)
read_message_ids = set(mail_read_statuses.values_list("message_id", flat=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

This set of message ids gets larger and larger, no?

Copy link
Contributor Author

@kevincarrogan kevincarrogan Jan 6, 2025

Choose a reason for hiding this comment

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

It does. This keeps the implementation mostly the same as it was before but switching to a set should mean a slightly faster lookup instead of building a big list and checking existence that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

So just to fully check my understanding, we get all of the IDs for the messages read since the start of time in the app? Any ideas how large this is right now?

Comment on lines 88 to 103
class MarkStatus:
def __init__(self, message_id, message_num, read_status):
self.message_id = message_id
self.message_num = message_num
self.read_status = read_status

def __call__(self, status):
logger.info(
"Marking message_id %s with message_num %s from %r as %s",
self.message_id,
self.message_num,
self.read_status.mailbox,
status,
)
self.read_status.status = status
self.read_status.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this just be a model method..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. But this is keeping the output of this function the same so I didn’t have to change it at the point where it’s used.

Comment on lines +111 to +112
def is_read(message, read_messages):
return message.message_id in read_messages
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if MailboxMessage can have the read status passed in to it on creation so that this could be a method on there instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this would end up being mostly the same in the end anyway, it’s just slightly shifting whether it’s a function or a method. You would still end up doing the same calls really.

message_id=message.message_id,
message_num=message.message_number,
mail_data=mail_data,
Copy link
Contributor

Choose a reason for hiding this comment

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

How can the email message data be mutable..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was to do with headers that came back from the mailbox. Technically the email is identical but there are headers that change when you read the message multiple times, something like a request id or an updating timestamp.

In this case it was to stop the message from being duplicated if the email gets read again as the data isn’t really changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I chose to do the update so it preserves the most recent headers in the case you did read the file data multiple times.

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.

2 participants