-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[No QA] Bootstrap secondary actions getter #57678
[No QA] Bootstrap secondary actions getter #57678
Conversation
@allgandalf Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
…ied-actions-get-secondary-action
return true; | ||
} | ||
|
||
const isPaymentProcessing = true; // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins I'm not sure how to determine this value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can check if isSettled === true
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins but the payment is processing and not settled ? do we assume processing payments as settled ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins current condition for showing cancel payment button is this:
App/src/pages/ReportDetailsPage.tsx
Line 367 in 237759d
const shouldShowCancelPaymentButton = caseID === CASES.MONEY_REPORT && canCancelPayment(moneyRequestReport, session); |
And canCancelPayment
implementation looks like this:
Lines 8287 to 8289 in 237759d
function canCancelPayment(iouReport: OnyxEntry<OnyxTypes.Report>, session: OnyxEntry<OnyxTypes.Session>) { | |
return isPayerReportUtils(session, iouReport) && (isSettled(iouReport) || iouReport?.isWaitingOnBankAccount) && isExpenseReport(iouReport); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins but the payment is processing and not settled ? do we assume processing payments as settled ?
Well, in this case the processing state is given by the combination of isSettled
and is past the NACHA cutoff (at which point we can't cancel it anymore. If we paid the report and we're within the nacha cutoff window, we can still cancel it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnowakow yes, we use isSettled
in the current condition. I think the isWaitingOnBankAccount
condition is redundant since isSettled
also takes that into account. The new change here is adding the nacha cutoff.
} | ||
|
||
const isPaymentProcessing = true; // TODO | ||
const hasDailyNachaCutoffPassed = false; // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins I'm not sure how to determine this value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cutoff is 23:45 PM UTC the day the payment was made
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so here I have to find payment transaction and get it's creation date and check if it's after 23:45 on that day?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can probably use getIOUActionForReportID to get the IOU action and check if it's a pay action and then use the reportAction date
|
||
const isIOU = isIOUReport(report); | ||
const hasWorkspaces = true; // TODO | ||
const isReceiver = true; // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins I'm not sure how to determine this value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the report.managerID
in the case of invoices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we're in context in IOU report but looking at onyx type I think it still will be mangerID
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that's still the same.
@jnowakow please also fix the failing test |
@@ -1265,7 +1265,7 @@ function isSettled(reportOrID: OnyxInputOrEntry<Report> | SearchReport | string | |||
return false; | |||
} | |||
|
|||
if (isEmptyObject(report) || report.isWaitingOnBankAccount) { | |||
if (isEmptyObject(report)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnowakow i don't understand the change here? why do we need to remove this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is contradiction in this function. Discussion is here
return isOpen || isProcessing || isApproved; | ||
} | ||
|
||
function getSecondaryAction( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnowakow shouldn't we just return if one of the conditions below are true? will there be a case where report will have many actions ? checking for each action isn't optimal right, unless i'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allgandalf assumption is that there will be only one primary action and it was already implemented here. There can be many secondary actions so we're creating array of them as stated in design doc.
It will look like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. We can have many secondary actions, which is what we're implementing in this PR.
…ied-actions-get-secondary-action
@jnowakow when do you think you can get this PR out of draft |
I think I will mark it as ready to review today |
Nice! Let's push to get this one reviewed today then! |
@luacmartins @allgandalf it's ready (I hope 🤞) |
@jnowakow quoting from the design doc:
Are we not doing it in this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnowakow please update the ReportSecondaryActionUtils
, it doesn't follow the style we follow in our codebase, i didn't comment on some functions but the same applies to all the functions, do let reach out to me if you need any extra help here, thanks !
import {allHavePendingRTERViolation, isDuplicate, isOnHold as isOnHoldTransactionUtils, shouldShowBrokenConnectionViolationForMultipleTransactions} from './TransactionUtils'; | ||
|
||
function isSubmitAction(report: Report, policy: Policy): boolean { | ||
const isExpense = isExpenseReport(report); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isExpense = isExpenseReport(report); | |
const isExpenseReport = isExpenseReportReportUtils(report); |
Please have a look at this slack post for details about when variable and util names are the same
return false; | ||
} | ||
|
||
const isSubmitter = isCurrentUserSubmitter(report.reportID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as ^
} | ||
|
||
const isSubmitter = isCurrentUserSubmitter(report.reportID); | ||
const isApprover = isApprovedMember(policy, getCurrentUserAccountID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use existing isApprover util here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using existing isApprover
from libs/actions/Policy/Member
return false; | ||
} | ||
|
||
const isOpen = isOpenReport(report); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as ^, please name the report as isOpenReport and adjust the util accordingly
} | ||
|
||
function isApproveAction(report: Report, policy: Policy, reportTransactions: Transaction[], violations: OnyxCollection<TransactionViolation[]>): boolean { | ||
const isExpense = isExpenseReport(report); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as ^
const isApproved = isReportApproved({report}); | ||
const isReportPayer = isPayer(getSession(), report, false, policy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as ^
const isApproved = isReportApproved({report}); | ||
const isReportPayer = isPayer(getSession(), report, false, policy); | ||
const isPaymentsEnabled = arePaymentsEnabled(policy); | ||
const isClosed = isClosedReport(report); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be names as isReportClosed
return true; | ||
} | ||
|
||
const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this check is correct, same @luacmartins we should check this with the current user right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
policy.role is specific to the current user, so this is comparing against the current user
} | ||
|
||
const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN; | ||
const isReimbursed = report.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isReportReimbursed please
const isReimbursed = report.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED; | ||
const syncEnabled = isAutoSyncEnabled(policy); | ||
const isReportExported = isExported(getReportActions(report)); | ||
const isFinished = isApproved || isReimbursed || isClosed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isFinished
doesn't really explain what this is used for, maybe something more meaningful?
@allgandalf we're gonna handle UI changes in a separate PR |
ohh, it confused me as it was written in the design doc section of |
@allgandalf I renamed all variables according to your suggestion |
Ready for another review from @allgandalf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, code LGTM! just one comment, but that won't block me from testing out this PR.
Completing checklist now
it('should always return default options', () => { | ||
const report = {} as unknown as Report; | ||
const policy = {} as unknown as Policy; | ||
// await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`, report); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: but lets remove this if we are not using it
// await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`, report); |
This PR has nothing to test in particular but i will try building each platform and verify that we are able to build successfully |
Reviewer Checklist
Screenshots/Videos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets ship this and clear out the commented code in a followup 🚀
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Explanation of Change
Adds logic for determining what secondary actions should be available for report.
Fixed Issues
$ #57438
PROPOSAL: N/A
Tests
Verify that
npm run test
passesOffline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop