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

[$250] Expense - In offline, expense created with violation red dot not shown in preview #54510

Open
2 of 8 tasks
IuliiaHerets opened this issue Dec 24, 2024 · 11 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Dec 24, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.78-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
Issue reported by: Applause Internal Team
Device used: Redmi note 10s Android 13
App Component: Money Requests

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Go to workspace settings
  3. Tap categories - upgrade it
  4. Enter payroll & gl code and save it
  5. Tap settings and enable members must categorize all expenses
  6. Open workspace chat
  7. Go offline
  8. Create an expense with big amount, enter merchant and set a future date
  9. Note expense preview shows no red dot
  10. Open the expense and note only missing category field displays red dot
  11. Go to workspace chat page
  12. Turn online
  13. Now note expense preview shows red dot
  14. Open the expense and note now red dot is shown in amount, receipt and for date field

Expected Result:

In offline, expense created with violation must show red dot.

Actual Result:

In offline, for expense created with violation, red dot is not shown in expense preview and for amount, receipt and for date field also red dot is not displayed in offline.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6701614_1735010104764.Screenrecorder-2024-12-24-08-18-59-291_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021871678007124043126
  • Upwork Job ID: 1871678007124043126
  • Last Price Increase: 2024-12-24
Issue OwnerCurrent Issue Owner: @s77rt
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 24, 2024
Copy link

melvin-bot bot commented Dec 24, 2024

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Copy link
Contributor

nacasim11 Your proposal will be dismissed because you did not follow the proposal template.

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 24, 2024

Edited by proposal-police: This proposal was edited at 2024-12-24 12:07:32 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Expense - In offline, expense created with violation red dot not shown in preview

What is the root cause of that problem?

We are not setting showInReview for the violations here

// Remove 'missingCategory' violation if category is valid according to policy
if (hasMissingCategoryViolation && isCategoryInPolicy) {
newTransactionViolations = reject(newTransactionViolations, {name: 'missingCategory'});
}
// Add 'missingCategory' violation if category is required and not set
if (!hasMissingCategoryViolation && policyRequiresCategories && !categoryKey) {
newTransactionViolations.push({name: 'missingCategory', type: CONST.VIOLATION_TYPES.VIOLATION});

What changes do you think we should make in order to solve the problem?

We should set showInReview same as how the BE sets

if (hasMissingCategoryViolation && isCategoryInPolicy) {
                newTransactionViolations = reject(newTransactionViolations, {name: 'missingCategory', showInReview: true});
            }

            // Add 'missingCategory' violation if category is required and not set
            if (!hasMissingCategoryViolation && policyRequiresCategories && !categoryKey) {
                newTransactionViolations.push({name: 'missingCategory', type: CONST.VIOLATION_TYPES.VIOLATION, showInReview: true});
            }

We should apply similar fixes for other violations where needed too like getTagViolationsForSingleLevelTags and getTagViolationsForDependentTags, customUnit

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can make a test for ViolationsUtils.getViolationsOnyxData to assert if showInReview is set properly

What alternative solutions did you explore? (Optional)

Copy link
Contributor

you did not follow the proposal template.

@daledah
Copy link
Contributor

daledah commented Dec 24, 2024

Edited by proposal-police: This proposal was edited at 2024-12-24 11:25:24 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

In offline, for expense created with violation, red dot is not shown in expense preview and for amount, receipt and for date field also red dot is not displayed in offline.

What is the root cause of that problem?

  1. We didn't enable showInReview when building violation optimistic data

newTransactionViolations.push({name: 'missingCategory', type: CONST.VIOLATION_TYPES.VIOLATION});

so the logic to detect violation of transaction is false here

(violation: TransactionViolation) => violation.type === CONST.VIOLATION_TYPES.VIOLATION && (showInReview === undefined || showInReview === (violation.showInReview ?? false)),

  1. We just have the logic to detect violation for category, not for amount and date

What changes do you think we should make in order to solve the problem?

  1. We should enable showInReview, we can alway put it as true or refer the BE logic
  2. For date and amount, we also need to build the optimistic data for it

Here is the sample code for date



// Parse the string into a Date object
const inputDate = new Date(updatedTransaction.created);

// Get the current date
const currentDate = new Date();

// Normalize both dates to ignore the time component
const normalizedInputDate = new Date(inputDate.getFullYear(), inputDate.getMonth(), inputDate.getDate());
const normalizedCurrentDate = new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate());

if (!hasFutureDateViolation && normalizedInputDate > normalizedCurrentDate) {
     newTransactionViolations.push({name: 'futureDate', type: CONST.VIOLATION_TYPES.VIOLATION, showInReview: true});
}

if (hasFutureDateViolation && normalizedInputDate <= normalizedCurrentDate) {
    newTransactionViolations = reject(newTransactionViolations, {name: 'futureDate'});
}

We can use the same logic for amount (overLimit violation) and receiptRequired (This logic above is just demo code, we can improve it by using lib,...)

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can test getViolationsOnyxData function to cover the overLimit, futureDate and others

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@bfitzexpensify bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Dec 24, 2024
@melvin-bot melvin-bot bot changed the title Expense - In offline, expense created with violation red dot not shown in preview [$250] Expense - In offline, expense created with violation red dot not shown in preview Dec 24, 2024
Copy link

melvin-bot bot commented Dec 24, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021871678007124043126

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 24, 2024
Copy link

melvin-bot bot commented Dec 24, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

@s77rt
Copy link
Contributor

s77rt commented Dec 25, 2024

@FitseTLT Thanks for the proposal. If showInReview is not set wouldn't that make this condition return true?

(violation: TransactionViolation) => violation.type === CONST.VIOLATION_TYPES.VIOLATION && (showInReview === undefined || showInReview === (violation.showInReview ?? false)),

@s77rt
Copy link
Contributor

s77rt commented Dec 25, 2024

@daledah Thanks for the proposal. I have the same question above ^

@s77rt
Copy link
Contributor

s77rt commented Dec 25, 2024

PS: I have also asked a related question here #51893 (comment)

@daledah
Copy link
Contributor

daledah commented Dec 25, 2024

@s77rt We're passing showInReview as true in

const hasViolations = TransactionUtils.hasViolation(transaction?.transactionID ?? '-1', transactionViolations, true);

so it just matches the violation that enable showInReview. Please tell me if any points are unclear. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

5 participants