-
Notifications
You must be signed in to change notification settings - Fork 368
Remove invoice interstitial modal #1505
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
Conversation
MayaRainer
left a comment
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.
@slavingia / @laugardie lmkwyt! test failures are on main as well
| await invoiceRow.click(); | ||
|
|
||
| await expect(page.getByRole("dialog")).toBeVisible(); | ||
| await page.getByRole("row").filter({ hasText: "Awaiting approval" }).click(); |
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.
Just simplified this a little bit.
| await page.getByRole("row").getByText("Awaiting approval").first().click(); | ||
| await page.getByRole("row").filter({ hasText: "Awaiting approval" }).click(); | ||
|
|
||
| await expect(page.getByRole("heading", { name: "INV-123456" })).toBeVisible(); |
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.
Asserting that the new page is visible rather than a modal.
| } | ||
| }; | ||
|
|
||
| const [rowSelection, setRowSelection] = useState<Record<string, boolean>>(() => { |
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.
To preserve the row selection when coming back from the detail page.
| await login(page, preOnboardingUser, `/invoices/${invoice.externalId}`); | ||
| await expect(page.getByText("Missing tax information.")).toBeVisible(); | ||
| await page.getByRole("link", { name: "Invoices" }).click(); | ||
| await page.getByRole("link", { name: "Invoices", exact: true }).click(); |
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.
To disambiguate from the "Back to invoices" link
MayaRainer
left a comment
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.
@laugardie _a Thanks! Could you have another look please?
|
@MayaRainer _a looks good to me! |
|
@laugardie Thanks! |
MayaRainer
left a comment
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.
| await page.getByPlaceholder("Description").first().fill("first item updated"); | ||
| await fillByLabel(page, "Hours / Qty", "04:30", { index: 0 }); | ||
| await expect(page.getByText("$870", { exact: true })).toBeVisible(); | ||
| await Promise.all([ |
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.
These checks were unnecessary and making the test flaky.
| </div> | ||
| ) : null} | ||
| {company.equityEnabled ? ( | ||
| <Totals |
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.
Made a component to share between the edit and show page as they're so similar now.
|
@MayaRainer just pushed some small UI changes. Rest looks good to me! I have one small concern regarding the UX when editing the equity (not strictly an issue with this PR, but related). If a contractor adds line items to a new invoice and then realizes they need to update the payment split, clicking that link navigates them away and they lose all the data they just entered. Even if open on a new tab, we need to reload to get the updated equity split. |
…xile into remove-invoice-interstitial
MayaRainer
left a comment
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.
maybe we could just save the form progress temporarily
Makes sense to me! Happy to make that change in a follow-up if we want to.
@ershad Good to merge?
| }, | ||
| expect: { timeout: 30000, toPass: { timeout: 30000 } }, | ||
| timeout: process.env.CI ? 30000 : 120000, | ||
| timeout: process.env.CI ? 60000 : 120000, |
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.
Increased this to 60s as the complete-flow spec is just so long that it sometimes can't complete within 30s. The expect timeout remains 30s, so any actually failing assertions will still fail after 30s.
ershad
left a comment
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.
@MayaRainer _a added a couple of comments, lgtm otherwise 👍🏼
| expensesTotal={invoice.expenses.reduce((acc, expense) => acc + expense.totalAmountInCents, BigInt(0))} | ||
| equityAmountInCents={invoice.equityAmountInCents} | ||
| equityNotice={ | ||
| invoice.equityAmountInCents > 0 ? ( |
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.
@MayaRainer just checked with @slavingia, for companies that have equity enabled, can we display the equity split and percentage even if it's 0?
| for company equity. | ||
| </span>{" "} | ||
| <Link href="/settings/payouts" className={linkClasses}> | ||
| Edit |
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.
Can we hide the link until we have a system to either save progress or automatically update the invoice numbers when the split is updated in settings?
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.
@ershad _a Thanks! Done and done, could you have another look please? Test failures look unrelated.
|
@ershad Gentle bump on this when you have a moment :) |
ershad
left a comment
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.
@MayaRainer added one comment, lgtm otherwise 👍🏼
Wasn't able to test with an equity invoice since the seed data doesn't include it. But it should be okay since tests cover that.
| </PrintTableHeader> | ||
| <PrintTableHeader className="w-[20%] text-right md:w-[15%] print:text-right"> | ||
| Cash rate | ||
| Rate |
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.
Could you make this Rate on web and Cash rate on print view?







Closes #1502.
Removes the interstitial modal when clicking on an invoice row, instead taking the user to the invoice page directly.
Before
Screencast_20251105_173455.webm
After
Screencast_20251105_182918.webm