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

CET-579/feat: Multiple invoice email support #62

Conversation

SalmanTwo
Copy link
Contributor

Feature branch for additional invoice emails input on order checkout

@SalmanTwo SalmanTwo requested a review from a team as a code owner January 27, 2025 16:21
@SalmanTwo SalmanTwo requested a review from brtkwr January 27, 2025 16:21
/**
* @inheritDoc
*/
public function isMultipleInvoiceEmailsEnabled(?int $storeId = null): bool
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: can we please drop the "Multiple" part? Gives that part too much emphasis IMO :D this is just about whether to enable invoice email or not.

*/
public function isMultipleInvoiceEmailsEnabled(?int $storeId = null): bool
{
return $this->isSetFlag(self::XML_PATH_ENABLE_MULTIPLE_INVOICE_EMAILS, $storeId);
Copy link
Member

Choose a reason for hiding this comment

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

Also drop MULTIPLE here

@@ -91,6 +91,7 @@ public function getConfig(): array
'checkoutPageUrl' => $this->configRepository->getCheckoutPageUrl(),
'redirectUrlCookieCode' => UrlCookie::COOKIE_NAME,
'isOrderIntentEnabled' => $this->configRepository->isOrderIntentEnabled(),
'isMultipleInvoiceEmailsEnabled' => $this->configRepository->isMultipleInvoiceEmailsEnabled(),
Copy link
Member

Choose a reason for hiding this comment

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

same here.

@@ -24,6 +24,7 @@ class DataAssignObserver extends AbstractDataAssignObserver
'department',
'orderNote',
'poNumber',
'multipleInvoiceEmails'
Copy link
Member

Choose a reason for hiding this comment

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

same here

<input
type="text"
id="multiple_invoice_emails"
data-bind="value: multipleInvoiceEmails, event: { change: validateMultipleEmails }"
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -172,6 +172,19 @@
/>
</div>
</div>
<div class="field field-text" data-bind="visible: isMultipleInvoiceEmailsEnabled">
<label for="multiple_invoice_emails" class="label">
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -392,7 +424,8 @@ define([
project: this.project(),
department: this.department(),
orderNote: this.orderNote(),
poNumber: this.poNumber()
poNumber: this.poNumber(),
multipleInvoiceEmails: this.multipleInvoiceEmails()
Copy link
Member

Choose a reason for hiding this comment

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

same here

}, duration);
}
},
validateMultipleEmails: function () {
Copy link
Member

Choose a reason for hiding this comment

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

validateInvoiceEmails

@@ -67,6 +69,7 @@ define([
autofillToken: '',
companyName: ko.observable(''),
companyId: ko.observable(''),
multipleInvoiceEmails: ko.observable(''),
Copy link
Member

Choose a reason for hiding this comment

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

invoiceEmails

etc/config.xml Outdated
@@ -18,6 +18,7 @@
<days_on_invoice>14</days_on_invoice>
<finalize_purchase>1</finalize_purchase>
<enable_order_intent>1</enable_order_intent>
<enable_multiple_invoice_emails>0</enable_multiple_invoice_emails>
Copy link
Member

Choose a reason for hiding this comment

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

enable_invoice_emails

@@ -158,6 +158,16 @@
</depends>
<config_path>payment/two_payment/enable_order_intent</config_path>
</field>
<field id="enable_multiple_invoice_emails" translate="label" type="select" sortOrder="75" showInDefault="1"
showInWebsite="1" showInStore="1">
<label>Enable multiple invoice emails</label>
Copy link
Member

Choose a reason for hiding this comment

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

Enable invoice emails

*
* @return bool
*/
public function isMultipleInvoiceEmailsEnabled(?int $storeId = null): bool;
Copy link
Member

Choose a reason for hiding this comment

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

isInvoiceEmailsEnabled

@@ -122,6 +123,15 @@ public function isCompanySearchEnabled(?int $storeId = null): bool;
*/
public function isOrderIntentEnabled(?int $storeId = null): bool;

/**
* Check if order intent is enabled
Copy link
Member

Choose a reason for hiding this comment

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

this text needs updating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with all other comments on the naming adjustment

@SalmanTwo SalmanTwo requested a review from brtkwr January 28, 2025 16:33
i18n/en_US.csv Outdated
@@ -113,3 +113,5 @@ Version,Version
"Your request to %1 failed. Reason: %2","Your request to %1 failed. Reason: %2"
"Your sole trader account could not be verified.","Your sole trader account could not be verified."
"Zip/Postal Code is not valid.","Zip/Postal Code is not valid."
"Forward invoice to email list (optional)","Forward invoice to email list (optional)"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this earlier, can we drop the (optional) and keep the wording similar to Woocom here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to match Woocom

@SalmanTwo SalmanTwo requested a review from brtkwr January 28, 2025 18:38
i18n/sv_SE.csv Outdated
@@ -113,3 +113,5 @@ Version,Version
"Your request to %1 failed. Reason: %2","Din begäran till %1 misslyckades. Orsak: %2"
"Your sole trader account could not be verified.","Ditt enskild näringsidkarekonto kunde inte verifieras."
"Zip/Postal Code is not valid.","Postnummer är inte giltigt."
"Invoice email address","E-postadress för faktura"
"Please ensure your forward to email list only contains valid emails seperated by commas.","Se till att din vidarebefordran till e-postlista endast innehåller giltiga e-postmeddelanden separerade med kommatecken."
Copy link
Member

Choose a reason for hiding this comment

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

can you please reword "Please ensure your forward to email list only contains valid emails seperated by commas" to "Please ensure that your invoice email address list only contains valid email addresses separated by commas"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@SalmanTwo SalmanTwo merged commit 128cf4c into main Jan 29, 2025
13 checks passed
@SalmanTwo SalmanTwo deleted the shabib/cet-579-adding-support-for-multiple-invoice-emails-in-magento branch January 29, 2025 15:43
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