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

Add document link to emails for guest customers #1039

Merged
merged 21 commits into from
Feb 7, 2025

Conversation

MohamadNateqi
Copy link
Contributor

close #1035

Copy link
Member

@alexmigf alexmigf left a comment

Choose a reason for hiding this comment

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

Some remarks:

  • This setting does not need to trigger a Preview refresh.
  • It might be useful to allow selecting the email template hook where the link is displayed.
  • Instead of disabling the field, we could display it only when the "Guest" access type is enabled. In this case, we would also need to update the field description.

includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Main.php Outdated Show resolved Hide resolved
includes/Endpoint.php Outdated Show resolved Hide resolved
includes/Main.php Show resolved Hide resolved
includes/Main.php Outdated Show resolved Hide resolved
includes/Main.php Outdated Show resolved Hide resolved
@MohamadNateqi
Copy link
Contributor Author

@alexmigf

It might be useful to allow selecting the email template hook where the link is displayed.

Do you suggest such a setting:
image
It's a multiselect to allow choosing several hooks.

Instead of disabling the field, we could display it only when the "Guest" access type is enabled. In this case, we would also need to update the field description.

I thought this way, customers would know that such a setting exists easily.
So, do you believe hiding it and adding a description for the access type setting to inform about this feature would be better?

@alexmigf
Copy link
Member

alexmigf commented Feb 4, 2025

It's a multiselect to allow choosing several hooks.

I believe a standard selector is more appropriate since the user is unlikely to select more than one option.

I thought this way, customers would know that such a setting exists easily.
So, do you believe hiding it and adding a description for the access type setting to inform about this feature would be better?

I understand your point, but I believe we should avoid promoting the "Guest" access type, as it is less secure. Customers already using "Guest" will see the option, and I believe they are the intended target.

Copy link
Member

@alexmigf alexmigf left a comment

Choose a reason for hiding this comment

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

In summary, I don't think we should use the terms "guest emails" and "Download invoice," as I believe neither fully reflects the actual concepts.

includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Main.php Outdated Show resolved Hide resolved
includes/Settings/SettingsDebug.php Outdated Show resolved Hide resolved
includes/Main.php Outdated Show resolved Hide resolved
includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Assets.php Outdated Show resolved Hide resolved
includes/Documents/Invoice.php Outdated Show resolved Hide resolved
includes/Main.php Outdated Show resolved Hide resolved
includes/Main.php Outdated Show resolved Hide resolved
includes/Main.php Outdated Show resolved Hide resolved
Copy link
Member

@alexmigf alexmigf left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexmigf alexmigf merged commit c597720 into main Feb 7, 2025
6 checks passed
@alexmigf alexmigf deleted the 1035-add-document-link-to-emails branch February 7, 2025 15:38
@alexmigf alexmigf restored the 1035-add-document-link-to-emails branch February 7, 2025 16:52
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.

Add Setting to Include Document Link in Email for Guest Customers
2 participants