Skip to content

use contact name in RecipientElement for sending#4275

Merged
tomholub merged 16 commits intomasterfrom
issue-4268-recipient-names
Feb 22, 2022
Merged

use contact name in RecipientElement for sending#4275
tomholub merged 16 commits intomasterfrom
issue-4268-recipient-names

Conversation

@rrrooommmaaa
Copy link
Contributor

This PR loads names to be added to email into RecipientElement

close #4268


Tests (delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@rrrooommmaaa
Copy link
Contributor Author

There are 3 places in the code to actually display names in recipient boxes. I can do this in this PR or later.
AddNameToEmail is only used for "From" now, Perhaps, we can eliminate it in #4270 ?

@rrrooommmaaa
Copy link
Contributor Author

Hi @tomholub
Though all tests pass, I'm a bit worried if I missed something. Do we need more tests for this?
I mainly added Names to RecipientElement, excluded 'wrong' addresses from evaluation.
Also, setEmailsPreview caused deadlock (after my changes somehow) when waiting for evaluation to finish, so now simply abort if evaluation is in progress, and is called again after each evaluation

@rrrooommmaaa
Copy link
Contributor Author

ping @tomholub

@rrrooommmaaa
Copy link
Contributor Author

image

@rrrooommmaaa
Copy link
Contributor Author

Name is now displayed name in "email preview" if available
image

@tomholub
Copy link
Collaborator

Hi @tomholub Though all tests pass, I'm a bit worried if I missed something. Do we need more tests for this? I mainly added Names to RecipientElement, excluded 'wrong' addresses from evaluation. Also, setEmailsPreview caused deadlock (after my changes somehow) when waiting for evaluation to finish, so now simply abort if evaluation is in progress, and is called again after each evaluation

I'll take a look.

As for the screenshots, they look good except the bolded part should have smaller weight. So instead of weight: bold could try weight: 300 or 400 or similar, to be somewhere in between.

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Quite a change this ended up being. It's not bad. Comments below

};

private addNamesToMsg = async (msg: SendableMsg): Promise<void> => {
private addNameToEmail = async (email: string): Promise<string> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's call this formatSenderEmailAsMimeString (is that what it does?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const signedData = await MsgUtil.sign(signingPrv, newMsg.plaintext);
const allContacts = [...newMsg.recipients.to || [], ...newMsg.recipients.cc || [], ...newMsg.recipients.bcc || []];
ContactStore.update(undefined, allContacts, { lastUse: Date.now() }).catch(Catch.reportErr);
const recipients = Value.arr.unique(Object.values(newMsg.recipients).reduce((a, b) => a.concat(b), []).map(x => x.email));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this line Value.arr.unique(Object.values(newMsg.recipients).reduce((a, b) => a.concat(b), []).map(x => x.email)); repeats a few times across the files, we could give it a name and make a method to describe what it does. I suppose getUniqueRecipientEmails ?

Copy link
Contributor Author

@rrrooommmaaa rrrooommmaaa Feb 21, 2022

Choose a reason for hiding this comment

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

that's right, I was thinking about this, but the type ParsedRecipients is too specific so any of the existing common/helper modules are not suitable. Perhaps, I should place the method to compose-types.ts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could try compose-recipients-module.ts as a public method, then access it in other modules as this.view.recipientsModule.uniqueRecipientEmails(newMsg.recipients) or similar.

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, let me fix it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh no... looks like recipientsModule isn't available from formatters

@rrrooommmaaa
Copy link
Contributor Author

As for the screenshots, they look good except the bolded part should have smaller weight. So instead of weight: bold could try weight: 300 or 400 or similar, to be somewhere in between.

is weight 500 looking good?
image

@tomholub
Copy link
Collaborator

Better 👍

@rrrooommmaaa rrrooommmaaa marked this pull request as ready for review February 21, 2022 16:08
@tomholub tomholub merged commit 70ba757 into master Feb 22, 2022
@tomholub tomholub deleted the issue-4268-recipient-names branch February 22, 2022 10:21
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.

include names in to/cc/bcc fields when calling FES

2 participants