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

Truncate email addresses that are more than 244 characters long #4708

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

StCyr
Copy link
Collaborator

@StCyr StCyr commented Mar 8, 2021

Fixes #3608

Signed-off-by: Cyrille Bollu [email protected]

Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

Code is fine - but what's the consequences for replying in the in the UI?

@StCyr
Copy link
Collaborator Author

StCyr commented Mar 9, 2021

Code is fine - but what's the consequences for replying in the in the UI?

Yes, that was one of my concerns: Replying just won't work.

I've added the "" substring to make it more clear to the user that the email address is not usable anymore.

But, let's be honest, emails coming from such addresses are most probably spam

@miaulalala
Copy link
Contributor

Code is fine - but what's the consequences for replying in the in the UI?

Yes, that was one of my concerns: Replying just won't work.

I've added the "" substring to make it more clear to the user that the email address is not usable anymore.

But, let's be honest, emails coming from such addresses are most probably spam

True. Is the "" substring clearly identifiable by the user?

@StCyr
Copy link
Collaborator Author

StCyr commented Mar 9, 2021

True. Is the "" substring clearly identifiable by the user?

(fun "" ("< t r u n c a t ed >") seems to have been truncated by Github)

Not much indeed but we could use it to show it differently in the frontend (in red? with an exclamation mark?)

@ChristophWurst
Copy link
Member

I'm not sold on the idea of a special string in the email address. Partly because that doesn't work with translations. Thinking of other issues where we can't fully read an email, e.g. when the encoding of the subject is problematic, there could be a has_errors flag or similar on a cached message in oc_mail_messages and when that is set to true we warn the user that some of the information displayed might be incorrect. Then we can more safely drop parts of the subject, truncate email addresses and so on. The downside is that we need a migration and that this migration can be slow.

@miaulalala
Copy link
Contributor

The time for a migration would be now.

I like the idea of having an error state for mails, this could be very useful.

@ChristophWurst
Copy link
Member

ChristophWurst commented Mar 11, 2021

@ChristophWurst ChristophWurst marked this pull request as draft March 11, 2021 10:27
@ChristophWurst ChristophWurst force-pushed the fix/3608/truncate_long_email_addresses branch from cd49e9c to bbba855 Compare June 27, 2023 18:07
@@ -341,7 +341,7 @@ public function insertBulk(Account $account, Message ...$messages): void {
$qb2->setParameter('message_id', $messageId, IQueryBuilder::PARAM_INT);
$qb2->setParameter('type', $type, IQueryBuilder::PARAM_INT);
$qb2->setParameter('label', mb_strcut($recipient->getLabel(), 0, 255), IQueryBuilder::PARAM_STR);
$qb2->setParameter('email', $recipient->getEmail(), IQueryBuilder::PARAM_STR);
$qb2->setParameter('email', mb_substr($recipient->getEmail(),0 , 255), IQueryBuilder::PARAM_STR);
Copy link
Member

Choose a reason for hiding this comment

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

adjusted to drop the <truncated> because it would be tricky with translations

@ChristophWurst ChristophWurst marked this pull request as ready for review June 27, 2023 18:08
@ChristophWurst ChristophWurst force-pushed the fix/3608/truncate_long_email_addresses branch from bbba855 to 58eef3d Compare June 27, 2023 18:11
@ChristophWurst ChristophWurst force-pushed the fix/3608/truncate_long_email_addresses branch from 58eef3d to c363cd0 Compare June 27, 2023 18:12
@ChristophWurst
Copy link
Member

/backport to stable3.3

@ChristophWurst
Copy link
Member

/backport to stable2.2

@ChristophWurst ChristophWurst merged commit 7cd36c5 into main Jun 27, 2023
@ChristophWurst ChristophWurst deleted the fix/3608/truncate_long_email_addresses branch June 27, 2023 18:17
@backportbot-nextcloud
Copy link

The backport to stable3.3 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable3.3
git pull origin stable3.3

# Create the new backport branch
git checkout -b fix/foo-stable3.3

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable3.3

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@ChristophWurst
Copy link
Member

/backport to stable3.2

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.

Too long email address breaks sync
3 participants