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.

Sign up for GitHub

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

feat: add card num and employee name #1086

Merged
merged 12 commits into from
Dec 9, 2024
Merged

feat: add card num and employee name #1086

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bf737a4
feat: add card num and employee name
anishfyle Nov 26, 2024
ed2cda3
lint fix
anishfyle Nov 26, 2024
427031c
formatmemoPrev
anishfyle Nov 26, 2024
d419882
add options card num and employee name across
anishfyle Nov 26, 2024
5d1707d
lint fix
anishfyle Nov 26, 2024
dfde884
handle export settings and app name
anishfyle Nov 26, 2024
3b459da
lint fix
anishfyle Nov 26, 2024
7db1c03
fix tests
anishfyle Nov 26, 2024
6b01914
pr comments
anishfyle Dec 5, 2024
b5b4877
Merge branch 'master' into add-card-num-emp-name
anishfyle Dec 5, 2024
87f4d0c
card_num pr comments
anishfyle Dec 9, 2024
e398a72
Merge branch 'master' into add-card-num-emp-name
anishfyle Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...siness-central/business-central-configuration/business-central-advanced-settings.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
export class BusinessCentralAdvancedSettingsModel {

static mapAPIResponseToFormGroup(advancedSettings: BusinessCentralAdvancedSettingsGet | null, isSkipExportEnabled: boolean): FormGroup {
const defaultMemoOptions: string[] = ['employee_email', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link'];
const defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number'];

Check failure on line 24 in src/app/core/models/business-central/business-central-configuration/business-central-advanced-settings.model.ts

View workflow job for this annotation

GitHub Actions / lint 

A space is required after ','
return new FormGroup({
memoStructure: new FormControl(advancedSettings?.expense_memo_structure ? advancedSettings?.expense_memo_structure : defaultMemoOptions),
scheduleEnabled: new FormControl(advancedSettings?.schedule_is_enabled ? true : false),
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/models/common/advanced-settings.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export type AdvancedSettingValidatorRule = {

export class AdvancedSettingsModel {
static getDefaultMemoOptions(): string[] {
return ['employee_email', 'merchant', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link'];
return ['employee_email', 'employee_name', 'merchant', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number'];
}

static formatMemoPreview(memoStructure: string[], defaultMemoOptions: string[]): [string, string[]] {
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/models/netsuite/netsuite-configuration/netsuite-advanced-settings.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export type NetsuiteAdvancedSettingAddEmailModel = {

export class NetsuiteAdvancedSettingModel extends HelperUtility {
static getDefaultMemoOptions(): string[] {
return ['employee_email', 'merchant', 'purpose', 'category', 'spent_on', 'report_number'];
return ['employee_email', 'employee_name', 'merchant', 'purpose', 'category', 'spent_on', 'report_number', 'card_number'];
anishfyle marked this conversation as resolved.
Show resolved Hide resolved
}

static getPaymentSyncOptions(): SelectFormOption[] {
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
export class Sage300AdvancedSettingModel {

static mapAPIResponseToFormGroup(advancedSettings: Sage300AdvancedSettingGet | null, isSkipExportEnabled: boolean): FormGroup {
const defaultMemoOptions: string[] = ['employee_email', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link'];
const defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number'];

Check failure on line 24 in src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts

View workflow job for this annotation

GitHub Actions / lint 

A space is required after ','
return new FormGroup({
memoStructure: new FormControl(advancedSettings?.memo_structure ? advancedSettings?.memo_structure : defaultMemoOptions),
scheduleEnabled: new FormControl(advancedSettings?.schedule_is_enabled ? true : false),
Expand Down
2 changes: 1 addition & 1 deletion ...shared/business-central-advanced-settings/business-central-advanced-settings.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@

ConfigurationCtaText = ConfigurationCta;

defaultMemoOptions: string[] = ['employee_email', 'purpose', 'category', 'spent_on', 'report_number'];
defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'card_number'];

Check failure on line 50 in src/app/integrations/business-central/business-central-shared/business-central-advanced-settings/business-central-advanced-settings.component.ts

View workflow job for this annotation

GitHub Actions / lint 

A space is required after ','
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistency found in memo options between components

The verification revealed that the Business Central component is missing 'expense_link' in its defaultMemoOptions array, which is present in both the Sage300 component and the model definitions:

  • src/app/integrations/business-central/business-central-shared/business-central-advanced-settings/business-central-advanced-settings.component.ts: Missing 'expense_link'
  • Other components and models include: ['employee_email', 'employee_name', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number']

Please align the defaultMemoOptions array with other components by adding the missing 'expense_link' option.

🔗 Analysis chain

LGTM! Addition of new memo fields.

The addition of 'employee_name' and 'card_number' to defaultMemoOptions aligns with the PR objectives. The component's existing memo preview and structure handling will automatically support these new fields.

Let's verify the integration of these new fields:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new memo fields are properly integrated across the codebase

# Check if the memo fields are consistently defined across components
rg -l "defaultMemoOptions.*employee_name.*card_number" --type ts

# Check for any related template updates for these fields
rg -l "employee_name|card_number" --type html

Length of output: 785

Script:

#!/bin/bash
# Let's verify the consistency of memo field implementations

# Check the actual implementation in Sage300 component
rg "defaultMemoOptions.*=.*\[.*\]" src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts

# Check the model definitions
rg "defaultMemoOptions" src/app/core/models/sage300/sage300-configuration/sage300-advanced-settings.model.ts
rg "defaultMemoOptions" src/app/core/models/business-central/business-central-configuration/business-central-advanced-settings.model.ts

# Check the HTML templates for memo field usage
rg -A 5 "employee_name|card_number" src/app/shared/components/si/helper/dashboard-intacct-errors/dashboard-intacct-errors.component.html
rg -A 5 "employee_name|card_number" src/app/shared/components/dashboard/dashboard-accounting-error-dialog/dashboard-accounting-error-dialog.component.html

Length of output: 2061

🧰 Tools
🪛 eslint

[error] 50-50: A space is required after ','.

(comma-spacing)

🪛 GitHub Check: lint

[failure] 50-50:
A space is required after ','


memoPreviewText: string;

Expand Down
2 changes: 2 additions & 0 deletions ...s/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ export class IntacctAdvancedSettingsComponent implements OnInit {

const previewValues: { [key: string]: string } = {
employee_email: '[email protected]',
employee_name: 'John Doe',
card_number: '1234 5678 9012 3456',
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security concern: Credit card number should be masked

The card number is currently displayed in full (1234 5678 9012 3456) in the memo preview. This could expose sensitive financial information and potentially violate PCI DSS requirements.

Apply this diff to mask the card number:

-      card_number: '1234 5678 9012 3456',
+      card_number: '•••• •••• •••• 3456',
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
employee_name: 'John Doe',
card_number: '1234 5678 9012 3456',
employee_name: 'John Doe',
card_number: '•••• •••• •••• 3456',

anishfyle marked this conversation as resolved.
Show resolved Hide resolved
category: 'Meals and Entertainment',
purpose: 'Client Meeting',
merchant: 'Pizza Hut',
Expand Down
2 changes: 2 additions & 0 deletions src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ export class QbdAdvancedSettingComponent implements OnInit {

const previewValues: { [key: string]: string } = {
employee_email: '[email protected]',
employee_name: 'John Doe',
card_number: '1234 5678 9012 3456',
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider masking sensitive card number information in preview

While adding card number to memo preview is useful, displaying the full card number might pose security risks. Consider:

  1. Masking the card number to show only the last 4 digits
  2. Adding a tooltip or note indicating this is a masked preview

Apply this diff to mask the card number:

-      card_number: '1234 5678 9012 3456',
+      card_number: '•••• •••• •••• 3456',
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
employee_name: 'John Doe',
card_number: '1234 5678 9012 3456',
employee_name: 'John Doe',
card_number: '•••• •••• •••• 3456',

anishfyle marked this conversation as resolved.
Show resolved Hide resolved
category: 'Meals and Entertainment',
purpose: 'Client Meeting',
merchant: 'Pizza Hut',
Expand Down
4 changes: 3 additions & 1 deletion ...s/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@

ConfigurationCtaText = ConfigurationCta;

defaultMemoOptions: string[] = ['employee_email', 'purpose', 'category', 'spent_on', 'report_number', 'expense_link'];
defaultMemoOptions: string[] = ['employee_email', 'employee_name','purpose', 'category', 'spent_on', 'report_number', 'expense_link', 'card_number'];

Check failure on line 55 in src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts

View workflow job for this annotation

GitHub Actions / lint 

A space is required after ','

memoPreviewText: string;

Expand Down Expand Up @@ -91,6 +91,8 @@

const previewValues: { [key: string]: string } = {
employee_email: '[email protected]',
employee_name: 'John Doe',
card_number: '1234 5678 9012 3456',
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Mask sensitive card number information

The card number is currently displayed in full in the preview, which could expose sensitive information. This needs to be addressed before merging.

-      card_number: '1234 5678 9012 3456',
+      card_number: '•••• •••• •••• 3456',

Consider creating a utility function to consistently mask card numbers across the application:

private maskCardNumber(cardNumber: string): string {
  if (!cardNumber) {
    return '';
  }
  const lastFourDigits = cardNumber.slice(-4);
  return `•••• •••• •••• ${lastFourDigits}`;
}

💡 Codebase verification

Based on the analysis of the codebase, I can now generate the final response:

Card numbers should be masked in preview/sample data

The hardcoded card numbers appear in preview/sample data across multiple integration components. While these are just example values, it's better to use masked formats even in preview data to promote secure practices.

  • Replace 1234 5678 9012 3456 with XXXX XXXX XXXX 3456 in:
    • src/app/integrations/sage300/sage300-shared/sage300-advanced-settings/sage300-advanced-settings.component.ts
    • src/app/integrations/qbd/qbd-shared/qbd-advanced-setting/qbd-advanced-setting.component.ts
    • src/app/integrations/intacct/intacct-shared/intacct-advanced-settings/intacct-advanced-settings.component.ts
    • src/app/core/models/common/advanced-settings.model.ts
🔗 Analysis chain

Verify secure handling of card numbers throughout the application

Let's ensure card numbers are properly masked everywhere they appear.

Let me gather more context about how these card numbers are being used.

Let me check one more thing to understand if these are just preview/sample values or actual card data.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential exposure of card numbers in code

# Search for card number patterns in typescript files
rg -t ts "card.*number.*[:\"'].*\d{4}.*\d{4}.*\d{4}.*\d{4}" 

# Search for potential logging of card numbers
rg -t ts "console\.log.*card.*number|log.*card.*number"

# Search for card number related variables and properties
ast-grep --pattern 'cardNumber: string'

Length of output: 694

Script:

#!/bin/bash
# Check the context where these card numbers appear
rg -B 5 -A 5 "card_number.*1234" 

# Look for any masking utilities or functions
rg -t ts "mask|hideCard|obscure"

# Check for any interfaces or types related to card data
ast-grep --pattern 'interface $_ {
  $$$
  card_number: $_
  $$$
}'

Length of output: 5882

Script:

#!/bin/bash
# Check for any preview or sample related context
rg -t ts "preview.*values|sample.*data" -B 2 -A 2

# Look for any actual card data handling
rg -t ts "card.*data|payment.*card|credit.*card" -B 2 -A 2

Length of output: 107902

category: 'Meals and Entertainment',
purpose: 'Client Meeting',
merchant: 'Pizza Hut',
Expand Down
Loading