From ebe5decb9600025e50ada2a287004b3d1955217e Mon Sep 17 00:00:00 2001 From: Nikhil Kothari Date: Mon, 7 Sep 2026 18:40:02 +0530 Subject: [PATCH] fix(banking): UI cleanup and better statement parsing (#58817) * fix(banking): reset scroll on searching accounts * fix(banking): show only past dates in date filter * fix(banking): clean up line heights and remove beta badge * fix(banking): show accurate count of import progress fix(banking): show latest 20 imports instead of 10 * fix(banking): layout sizing needs to be preserved on page change * fix(banking): cleaner bank balance UI * fix(banking): correctly parse Cr/Dr values in statement importer * Update banking/src/components/features/BankReconciliation/BankBalance.tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> --- .../components/common/AccountsDropdown.tsx | 7 +- .../components/common/LinkFieldCombobox.tsx | 9 +- .../BankReconciliation/BankBalance.tsx | 307 ++++++++----- .../BankClearanceSummary.tsx | 9 +- .../BankReconciliation/BankPicker.tsx | 11 +- .../BankReconciliation/BankRecDateFilter.tsx | 434 +++++++++++------- .../BankReconciliationStatement.tsx | 14 +- .../BankTransactionList.tsx | 9 +- .../IncorrectlyClearedEntries.tsx | 12 +- .../BankReconciliation/MatchAndReconcile.tsx | 58 ++- .../SelectedTransactionDetails.tsx | 4 +- .../TransferModalContent.tsx | 2 +- .../CSV/StatementDetails.tsx | 11 +- banking/src/components/ui/list-view.tsx | 2 +- banking/src/hooks/useFiscalYear.ts | 63 ++- banking/src/hooks/useResetScrollOnSearch.ts | 23 + banking/src/index.css | 1 + banking/src/pages/BankReconciliation.tsx | 106 +++-- banking/src/pages/BankStatementImporter.tsx | 2 +- banking/src/styles/scroll-fade.css | 94 ++++ .../bank_statement_import_log.py | 170 +++++-- .../test_bank_statement_import_log.py | 251 +++++++++- 22 files changed, 1162 insertions(+), 437 deletions(-) create mode 100644 banking/src/hooks/useResetScrollOnSearch.ts create mode 100644 banking/src/styles/scroll-fade.css diff --git a/banking/src/components/common/AccountsDropdown.tsx b/banking/src/components/common/AccountsDropdown.tsx index a98ace578c3..6bf23872fde 100644 --- a/banking/src/components/common/AccountsDropdown.tsx +++ b/banking/src/components/common/AccountsDropdown.tsx @@ -9,6 +9,7 @@ import Fuse from "fuse.js" import { ChevronDownIcon } from "lucide-react" import { useLayoutEffect, useMemo, useRef, useState } from "react" import { FormControl } from "../ui/form" +import useResetScrollOnSearch from "@/hooks/useResetScrollOnSearch" export interface AccountsDropdownProps { @@ -104,6 +105,10 @@ const AccountsDropdown = ({ root_type, report_type, account_type, value, onChang const buttonRef = useRef(null) + // Searching replaces the grouped list with a short result list, so pin the scroll back to + // the top - otherwise the auto-selected first result can be out of view. + const listRef = useResetScrollOnSearch(search) + const [width, setWidth] = useState(320) useLayoutEffect(() => { @@ -153,7 +158,7 @@ const AccountsDropdown = ({ root_type, report_type, account_type, value, onChang - + {_("No accounts found.")} {recommendedAccounts.length > 0 && ( diff --git a/banking/src/components/common/LinkFieldCombobox.tsx b/banking/src/components/common/LinkFieldCombobox.tsx index a41105b05d7..a486f6286c3 100644 --- a/banking/src/components/common/LinkFieldCombobox.tsx +++ b/banking/src/components/common/LinkFieldCombobox.tsx @@ -10,6 +10,7 @@ import { ChevronDownIcon, ExternalLink } from "lucide-react"; import { Button } from "../ui/button"; import { cn } from "@/lib/utils"; import { Command, CommandEmpty, CommandGroup, CommandInput, CommandItem, CommandList } from "../ui/command"; +import useResetScrollOnSearch from "@/hooks/useResetScrollOnSearch"; import _ from "@/lib/translate"; import ErrorBanner from "../ui/error-banner"; import MarkdownRenderer from "../ui/markdown"; @@ -149,6 +150,10 @@ const LinkFieldCombobox = ({ const buttonRef = useRef(null) + // Results change as the search runs, so pin the scroll back to the top to keep the + // auto-selected first result in view. + const listRef = useResetScrollOnSearch(searchInput) + const [width, setWidth] = useState(320) useLayoutEffect(() => { @@ -264,7 +269,7 @@ const LinkFieldCombobox = ({ {error && } - + {isLoading ? _("Loading...") : _("No results found.")} {items?.map((result) => ( @@ -272,7 +277,7 @@ const LinkFieldCombobox = ({ {result.label || result.value} - {result.description && + {result.description && } diff --git a/banking/src/components/features/BankReconciliation/BankBalance.tsx b/banking/src/components/features/BankReconciliation/BankBalance.tsx index 632f3d62e8d..a0b9b0e3160 100644 --- a/banking/src/components/features/BankReconciliation/BankBalance.tsx +++ b/banking/src/components/features/BankReconciliation/BankBalance.tsx @@ -6,13 +6,13 @@ import { Progress } from "@/components/ui/progress" import { useGetAccountClosingBalance, useGetAccountClosingBalanceAsPerStatement, useGetAccountOpeningBalance, useGetUnreconciledTransactions } from "./utils" import { flt, formatCurrency } from "@/lib/numbers" import { Skeleton } from "@/components/ui/skeleton" -import { StatContainer, StatLabel, StatValue } from "@/components/ui/stats" import { Edit, Info, Trash2 } from "lucide-react" import { H4, Paragraph } from "@/components/ui/typography" import { HoverCard, HoverCardContent, HoverCardTrigger } from "@/components/ui/hover-card" import { getCompanyCurrency } from "@/lib/company" import _ from "@/lib/translate" -import { Dialog, DialogClose, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle, DialogTrigger } from "@/components/ui/dialog" +import { cn } from "@/lib/utils" +import { Dialog, DialogClose, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from "@/components/ui/dialog" import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip" import { formatDate } from "@/lib/date" import { Form } from "@/components/ui/form" @@ -26,50 +26,109 @@ import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from "@ import { toast } from "sonner" import ErrorBanner from "@/components/ui/error-banner" -const BankBalance = () => { +const useBankCurrency = () => { + const bankAccount = useAtomValue(selectedBankAccountAtom) + return bankAccount?.account_currency ?? getCompanyCurrency(bankAccount?.company ?? '') +} + +/** + * One line of the balance summary - label on the left, figure right-aligned. + * + * `items-baseline` keeps the figure on the label's FIRST line, so a row carrying a `subLabel` + * (the statement row's "As of " note) doesn't centre its value against both lines. + */ +const BalanceRow = ({ label, info, subLabel, emphasis, children }: { + label: React.ReactNode + info?: React.ReactNode + subLabel?: React.ReactNode + emphasis?: boolean + children: React.ReactNode +}) => ( +
+ + + {label} + {info} + + {subLabel} + +
{children}
+
+) + +/** + * Type styles for a figure. Shared so an interactive figure can put them on the + + {tooltip} + + } + subLabel={!isDateSame && data?.message.date + ? + {_("As of {0}", [formatDate(data?.message?.date ?? '', 'Do MMM YYYY')])} + + : undefined} + > + {/* Deliberately NOT a flex container: a flex box's baseline doesn't resolve to its + text, so the row's `items-baseline` couldn't line this up with the label. As a + plain inline button its baseline is the figure's own, like every other row. + "Set" gets the same treatment as a figure - it stands in for one. */} + {isLoading + ? + : + + {/* The figure styles live on the button itself - see + BALANCE_VALUE_CLASSES. `p-0` because preflight leaves the UA's + button padding in place. */} + + + {tooltip} + } + + + + setIsOpen(false)} + /> + + + + ) +} + +const DifferenceRow = () => { + const bankAccount = useAtomValue(selectedBankAccountAtom) + const currency = useBankCurrency() const { data, isLoading } = useGetAccountClosingBalance() @@ -102,16 +257,15 @@ const Difference = () => { const isError = difference !== 0 - return - {_("Difference")} - {isLoading ? : - {formatCurrency(difference, - bankAccount?.account_currency ?? getCompanyCurrency(bankAccount?.company ?? '')) - }} - + return + {isLoading + ? + : {formatCurrency(difference, currency)}} + } -const ReconcileProgress = () => { +/** Reconciliation progress through the selected date range: a count plus a slim bar. */ +const ReconciledRow = () => { const bankAccount = useAtomValue(selectedBankAccountAtom) @@ -132,75 +286,14 @@ const ReconcileProgress = () => { const progress = (totalCount ? reconciledCount / totalCount : 0) * 100 - return
-
- -
+ return
+ + {reconciledCount} / {totalCount ?? 0} + +
} -const ClosingBalanceAsPerStatement = () => { - - const bankAccount = useAtomValue(selectedBankAccountAtom) - const dates = useAtomValue(bankRecDateAtom) - const setValue = useSetAtom(bankRecClosingBalanceAtom(bankAccount?.name ?? '')) - - const { data, isLoading } = useGetAccountClosingBalanceAsPerStatement({ - onSuccess: (data) => { - if (data?.message && data?.message?.balance) { - setValue({ - value: data?.message?.balance, - stringValue: data?.message?.balance.toString() - }) - } - } - }) - - const isDateSame = data?.message?.date === dates.toDate - - const [isOpen, setIsOpen] = useState(false) - - - return - {_("Closing Balance as per statement")} -
- - - - -
- {isLoading ? : {formatCurrency(flt(data?.message?.balance, 2), bankAccount?.account_currency ?? getCompanyCurrency(bankAccount?.company ?? ''))}} - -
- - - {_("Click to set the closing balance as per statement")} - - - - - setIsOpen(false)} - /> - - - - - {!isDateSame && data?.message.date && {_("As of {0}", [formatDate(data?.message?.date ?? '', 'Do MMM YYYY')])}} -
- - -} - const ClosingBalanceForm = ({ defaultBalance, date, bankAccount, onClose }: { defaultBalance: number, date: string, bankAccount: SelectedBank | null, onClose: VoidFunction }) => { const { mutate } = useSWRConfig() @@ -302,7 +395,7 @@ const ClosingBalancesList = ({ bankAccount, date }: { bankAccount: SelectedBank return
-

{_("Balances as per bank statement before {0}", [formatDate(date, 'Do MMM YYYY')])}

+

{_("Balances as per bank statement before {0}", [formatDate(date, 'Do MMM YYYY')])}

@@ -331,4 +424,4 @@ const ClosingBalancesList = ({ bankAccount, date }: { bankAccount: SelectedBank } -export default BankBalance \ No newline at end of file +export default BankAccountBalancePanel diff --git a/banking/src/components/features/BankReconciliation/BankClearanceSummary.tsx b/banking/src/components/features/BankReconciliation/BankClearanceSummary.tsx index c26b9e9fb22..4c44507b2ba 100644 --- a/banking/src/components/features/BankReconciliation/BankClearanceSummary.tsx +++ b/banking/src/components/features/BankReconciliation/BankClearanceSummary.tsx @@ -205,9 +205,9 @@ const BankClearanceSummaryView = () => { const content = _("Below is a list of all accounting entries posted against the bank account {0} between {1} and {2}.", [`${bankAccount?.account}`, `${formattedFromDate}`, `${formattedToDate}`]) - return
+ return
-
+
@@ -220,8 +220,9 @@ const BankClearanceSummaryView = () => { data={data.message.result} columns={clearanceColumns} getRowId={(row) => `${row.payment_entry}-${row.posting_date}`} - maxHeight="calc(100vh - 200px)" - scrollAreaClassName="min-h-[calc(100vh-200px)]" + className="min-h-0 flex-1" + maxHeight="none" + scrollAreaClassName="flex-1" emptyState={_("No rows to display.")} /> ) : null} diff --git a/banking/src/components/features/BankReconciliation/BankPicker.tsx b/banking/src/components/features/BankReconciliation/BankPicker.tsx index 47b087bfa81..a5c65402ec2 100644 --- a/banking/src/components/features/BankReconciliation/BankPicker.tsx +++ b/banking/src/components/features/BankReconciliation/BankPicker.tsx @@ -74,7 +74,10 @@ const BankPicker = ({ className }: { className?: string }) => { } return (
4 ? 'pb-2' : '', className, )} style={{ @@ -108,12 +111,12 @@ const BankPickerItem = ({ bank }: { bank: SelectedBank }) => { role="button" title={`Select ${bank.account_name}`} onClick={onSelect} - className={cn('rounded-md border border-outline-gray-1 max-w-60 min-w-60 p-2 overflow-hidden cursor-pointer', + // `shrink-0`: this is a horizontally scrolling row, so cards keep their own width + // instead of being compressed to fit the container. + className={cn('w-60 shrink-0 rounded-md border border-outline-gray-1 p-2 overflow-hidden cursor-pointer transition-colors', isSelected ? 'border-outline-gray-5 bg-surface-gray-1' : 'hover:bg-surface-gray-1' )} > - -
diff --git a/banking/src/components/features/BankReconciliation/BankRecDateFilter.tsx b/banking/src/components/features/BankReconciliation/BankRecDateFilter.tsx index 84bd5278ccc..0cf530e1c6b 100644 --- a/banking/src/components/features/BankReconciliation/BankRecDateFilter.tsx +++ b/banking/src/components/features/BankReconciliation/BankRecDateFilter.tsx @@ -5,107 +5,179 @@ import { AVAILABLE_TIME_PERIODS, formatDate, getDatesForTimePeriod, TimePeriod } import { Button } from '@/components/ui/button' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' import { ChevronDownIcon, ChevronLeftIcon, ChevronRight } from 'lucide-react' -import { Command, CommandEmpty, CommandInput, CommandItem, CommandList } from '@/components/ui/command' +import { Command, CommandGroup, CommandInput, CommandItem, CommandList } from '@/components/ui/command' import { parse } from "chrono-node" import { Calendar } from '@/components/ui/calendar' import useFiscalYear from '@/hooks/useFiscalYear' import dayjs from 'dayjs' import _ from '@/lib/translate' import { useDirection } from '@/components/ui/direction' +import useResetScrollOnSearch from '@/hooks/useResetScrollOnSearch' + +const DATE_FORMAT = 'YYYY-MM-DD' + +/** Current fiscal year plus this many previous ones, for quarter/year options. */ +const PREVIOUS_FISCAL_YEARS = 2 + +type DateOption = { + /** Stable id - used as the cmdk value and the React key. */ + key: string + label: string + translatedLabel: string + fromDate: string + toDate: string + format: string + /** Extra terms to match against, beyond the labels and dates. */ + keywords?: string[] + /** Whether to show this option when the search box is empty. */ + isDefault?: boolean +} + +/** + * Fiscal years keep the same month/day boundaries year on year, so previous years can be + * derived by subtracting whole years instead of fetching them. Works for both Jan-Dec and + * Apr-Mar style fiscal years. + */ +const fiscalYearLabel = (start: dayjs.Dayjs, end: dayjs.Dayjs) => + start.year() === end.year() ? `${start.year()}` : `${start.year()}-${end.year()}` const BankRecDateFilter = () => { const [bankRecDate, setBankRecDate] = useAtom(bankRecDateAtom) - const { data: fiscalYear } = useFiscalYear() + const { fiscalYear } = useFiscalYear() - const timePeriodOptions = useMemo(() => { - const standardOptions = AVAILABLE_TIME_PERIODS.map((period) => { + const today = useMemo(() => dayjs().format(DATE_FORMAT), []) + + const allOptions = useMemo(() => { + const standardOptions: DateOption[] = AVAILABLE_TIME_PERIODS.map((period) => { const dates = getDatesForTimePeriod(period) return { + key: period, label: period, + translatedLabel: dates.translatedLabel ?? _(period), fromDate: dates.fromDate, toDate: dates.toDate, format: dates.format, - translatedLabel: dates.translatedLabel + isDefault: true, } }) - if (fiscalYear?.message) { - // For a fiscal year, we need to replace "Last Year", "This Year", and add options for quarters - const fiscalYearStart = fiscalYear.message.year_start_date - const fiscalYearEnd = fiscalYear.message.year_end_date - - const q1 = { - label: `Q1: ${fiscalYear.message.name}`, - translatedLabel: `${_("Q1")}: ${fiscalYear.message.name}`, - fromDate: fiscalYearStart, - toDate: dayjs(fiscalYearStart).add(3, 'month').format('YYYY-MM-DD'), - format: 'MMM YYYY' - } - - const q2 = { - label: `Q2: ${fiscalYear.message.name}`, - translatedLabel: `${_("Q2")}: ${fiscalYear.message.name}`, - fromDate: dayjs(fiscalYearStart).add(3, 'month').format('YYYY-MM-DD'), - toDate: dayjs(fiscalYearStart).add(6, 'month').format('YYYY-MM-DD'), - format: 'MMM YYYY' - } - - const q3 = { - label: `Q3: ${fiscalYear.message.name}`, - translatedLabel: `${_("Q3")}: ${fiscalYear.message.name}`, - fromDate: dayjs(fiscalYearStart).add(6, 'month').format('YYYY-MM-DD'), - toDate: dayjs(fiscalYearStart).add(9, 'month').format('YYYY-MM-DD'), - format: 'MMM YYYY' - } - - const q4 = { - label: `Q4: ${fiscalYear.message.name}`, - translatedLabel: `${_("Q4")}: ${fiscalYear.message.name}`, - fromDate: dayjs(fiscalYearStart).add(9, 'month').format('YYYY-MM-DD'), - toDate: fiscalYearEnd, - format: 'MMM YYYY' - } - - const thisYear = { - label: `This Fiscal Year`, - translatedLabel: `${_("This Fiscal Year")}`, - fromDate: fiscalYearStart, - toDate: fiscalYearEnd, - format: 'MMM YYYY' - } - - const lastYear = { - label: `Last Fiscal Year`, - translatedLabel: `${_("Last Fiscal Year")}`, - fromDate: dayjs(fiscalYearStart).subtract(1, 'year').format('YYYY-MM-DD'), - toDate: dayjs(fiscalYearEnd).subtract(1, 'year').format('YYYY-MM-DD'), - format: 'MMM YYYY' - } - // Sort the options so that we get "This Month", "Last Month", quarters, fiscal year, then the rest of the standard options - - const topRankedItems = standardOptions.filter((option) => { - return option.label === "This Month" || option.label === "Last Month" - }) - - const bottomRankedItems = standardOptions.filter((option) => { - return option.label !== "This Month" && option.label !== "Last Month" - }) - - return [...topRankedItems, q1, q2, q3, q4, thisYear, lastYear, ...bottomRankedItems] + if (!fiscalYear) { + return standardOptions } - return standardOptions + const currentStart = dayjs(fiscalYear.year_start_date) + const currentEnd = dayjs(fiscalYear.year_end_date) + + const quarterOptions: DateOption[] = [] + const fiscalYearOptions: DateOption[] = [] + + // Static literals so the translation extractor can find them. + const quarterLabels = [_("Q1"), _("Q2"), _("Q3"), _("Q4")] + + for (let yearsAgo = 0; yearsAgo <= PREVIOUS_FISCAL_YEARS; yearsAgo++) { + const start = currentStart.subtract(yearsAgo, 'year') + const end = currentEnd.subtract(yearsAgo, 'year') + // Keep the real name for the current year; derive it for the earlier ones. + const yearLabel = yearsAgo === 0 ? fiscalYear.name : fiscalYearLabel(start, end) + + for (let quarter = 0; quarter < 4; quarter++) { + const quarterStart = start.add(quarter * 3, 'month') + // End the day before the next quarter starts, clamped to the fiscal year end + // so a short fiscal year can't spill over. + const nextQuarterStart = start.add((quarter + 1) * 3, 'month') + const quarterEnd = nextQuarterStart.subtract(1, 'day').isAfter(end) + ? end + : nextQuarterStart.subtract(1, 'day') + + if (quarterStart.isAfter(end)) continue + + quarterOptions.push({ + key: `Q${quarter + 1}-${yearLabel}`, + label: `Q${quarter + 1}: ${yearLabel}`, + translatedLabel: `${quarterLabels[quarter]}: ${yearLabel}`, + fromDate: quarterStart.format(DATE_FORMAT), + toDate: quarterEnd.format(DATE_FORMAT), + format: 'MMM YYYY', + keywords: ['quarter', `q${quarter + 1}`, yearLabel], + // Only the current fiscal year's quarters clutter the default list; + // older ones stay searchable. + isDefault: yearsAgo === 0, + }) + } + + const label = yearsAgo === 0 + ? 'This Fiscal Year' + : yearsAgo === 1 + ? 'Last Fiscal Year' + : `FY ${yearLabel}` + + fiscalYearOptions.push({ + key: `fiscal-year-${yearLabel}`, + label, + translatedLabel: yearsAgo <= 1 ? _(label) : `${_("FY")} ${yearLabel}`, + fromDate: start.format(DATE_FORMAT), + toDate: end.format(DATE_FORMAT), + format: 'MMM YYYY', + keywords: ['fiscal year', yearLabel], + isDefault: yearsAgo <= 1, + }) + } + + // "This Month"/"Last Month" first, then quarters and fiscal years, then the rest. + const topRanked = standardOptions.filter((o) => o.label === 'This Month' || o.label === 'Last Month') + const bottomRanked = standardOptions.filter((o) => o.label !== 'This Month' && o.label !== 'Last Month') + + return [...topRanked, ...quarterOptions, ...fiscalYearOptions, ...bottomRanked] }, [fiscalYear]) + // Reconciliation only looks backwards, so a period that hasn't started is never useful. + const selectableOptions = useMemo( + () => allOptions.filter((option) => option.fromDate <= today), + [allOptions, today], + ) + const [open, setOpen] = useState(false) const [value, setValue] = useState("") + // We filter ourselves (`shouldFilter={false}`) so that the parsed-date suggestion can be a + // real CommandItem alongside the predefined options, and keyboard navigation covers both. + const filteredOptions = useMemo(() => { + const query = value.trim().toLowerCase() + + if (!query) { + return selectableOptions.filter((option) => option.isDefault) + } + + const tokens = query.split(/\s+/) + + return selectableOptions.filter((option) => { + const haystack = [ + option.label, + option.translatedLabel, + ...(option.keywords ?? []), + option.fromDate, + option.toDate, + ].join(' ').toLowerCase() + + return tokens.every((token) => haystack.includes(token)) + }) + }, [selectableOptions, value]) + + const parsedOption = useMemo(() => parseDateRange(value), [value]) + + // Filtering shortens the list, so pin the scroll back to the top to keep the + // auto-selected first option in view. + const listRef = useResetScrollOnSearch(value) + + // Don't show a parsed suggestion that duplicates an option already in the list. + const showParsedOption = parsedOption + && !filteredOptions.some((o) => o.fromDate === parsedOption.fromDate && o.toDate === parsedOption.toDate) + const timePeriod: TimePeriod | string = useMemo(() => { if (bankRecDate.fromDate && bankRecDate.toDate) { - // Check if the from and to dates match any predefined time period - for (const period of timePeriodOptions) { + for (const period of allOptions) { if (period.fromDate === bankRecDate.fromDate && period.toDate === bankRecDate.toDate) { return period.label; } @@ -114,10 +186,11 @@ const BankRecDateFilter = () => { } else { return "Date Range"; } - }, [bankRecDate.fromDate, bankRecDate.toDate, timePeriodOptions]); + }, [bankRecDate.fromDate, bankRecDate.toDate, allOptions]); const handleTimePeriodChange = (fromDate: string, toDate: string) => { setBankRecDate({ fromDate, toDate }) + setValue("") setOpen(false) } @@ -130,7 +203,9 @@ const BankRecDateFilter = () => { const direction = useDirection() - + const RangeArrow = direction === 'ltr' + ? + : return
@@ -141,30 +216,57 @@ const BankRecDateFilter = () => { size='md' className='rounded-e-none border-e-0' role="combobox"> - {timePeriodOptions.find((period) => period.label === timePeriod)?.translatedLabel ?? _(timePeriod)} + {allOptions.find((period) => period.label === timePeriod)?.translatedLabel ?? _(timePeriod)} - + - - - - - - {timePeriodOptions.map((period) => ( - handleTimePeriodChange(period.fromDate, period.toDate)}> - - {period.translatedLabel ?? _(period.label)} - - - {formatDate(period.fromDate, period.format)} {direction === 'ltr' ? : } {formatDate(period.toDate, period.format)} - - - ))} + + + {showParsedOption && parsedOption && ( + + handleTimePeriodChange(parsedOption.fromDate, parsedOption.toDate)}> + {value} + + {parsedOption.fromDate === parsedOption.toDate + ? formatDate(parsedOption.fromDate, 'Do MMM YYYY') + : <>{formatDate(parsedOption.fromDate, 'Do MMM YY')} {RangeArrow} {formatDate(parsedOption.toDate, 'Do MMM YY')}} + + + + )} + + {filteredOptions.length > 0 && ( + + {filteredOptions.map((period) => ( + handleTimePeriodChange(period.fromDate, period.toDate)}> + + {period.translatedLabel} + + + {formatDate(period.fromDate, period.format)} {RangeArrow} {formatDate(period.toDate, period.format)} + + + ))} + + )} + + {!showParsedOption && filteredOptions.length === 0 && ( +
+ {_("No results found")} +
+ )} @@ -199,77 +301,97 @@ const BankRecDateFilter = () => { } const referentialKeywords = ["last", "this", "next", "previous"] -const EmptyState = ({ onSelect, value }: { onSelect: (fromDate: string, toDate: string) => void, value: string }) => { - const dates = useMemo(() => { - if (value) { - // Try parsing the value - const parsedDate = parse(value, undefined, { forwardDate: false }) +/** chrono exposes `knownValues` on ParsingComponents but doesn't type it publicly. */ +const knownValuesOf = (components: unknown): Record => + (components as { knownValues?: Record })?.knownValues ?? {} - if (parsedDate && parsedDate.length > 0) { - const startDate = parsedDate[0].start.date() - const endDate = parsedDate[0].end?.date() +/** + * How far back a parsed date must move to land in the past. Reconciliation only ever looks + * backwards, so an ambiguous input that chrono resolves into the future - "December" typed in + * September, or a bare weekday like "Friday" - is pulled to its most recent past occurrence. + * An explicitly stated year is respected; a range that is still future gets discarded later. + * + * This returns a shift rather than a date so that a range can be moved as a single unit - + * shifting its start and end independently would distort or invert it. + */ +const pastShift = (date: Date, knownValues: Record) => { + const today = dayjs() + let candidate = dayjs(date) - if (!endDate) { - const today = new Date() - // If today is greater than the start date, use today as the end date - if (startDate.getTime() > today.getTime()) { - return { fromDate: today, toDate: startDate } - } else { - // Check if the user only wants a specific month like "May 2025" - // If the "known values" just has month and year, then we need to get the first day of the month and the last day of the month - // @ts-expect-error - "Known Values" is available in the start "ParsingComponents" - if (parsedDate[0].start.knownValues?.month && !parsedDate[0].start.knownValues?.day) { - return { - fromDate: startDate, - toDate: dayjs(startDate).endOf('month').toDate() - } - // @ts-expect-error - "Known Values" is available in the start "ParsingComponents" - } else if (parsedDate[0].start.knownValues?.month && parsedDate[0].start.knownValues?.day && !referentialKeywords.some(keyword => value.toLowerCase().includes(keyword))) { - // If month and day is known, then we should not assume that the user wants to get everything until today - return { - fromDate: startDate, - toDate: startDate, - } - } - - return { - fromDate: startDate, - toDate: today - } - } - } else { - return { fromDate: startDate, toDate: endDate } - } - } - - } - }, [value]) - - const onClick = (fromDate: Date, toDate: Date) => { - onSelect(formatDate(fromDate, 'YYYY-MM-DD'), formatDate(toDate, 'YYYY-MM-DD')) + if (!candidate.isAfter(today, 'date') || knownValues.year !== undefined) { + return { amount: 0, unit: 'year' as const } } - const isEqual = dates?.fromDate && dates?.toDate && dayjs(dates.fromDate).isSame(dates.toDate, 'date') + // A bare weekday repeats weekly, everything else (month/day) repeats yearly. + const unit = knownValues.weekday !== undefined && knownValues.day === undefined + ? 'day' as const + : 'year' as const + const step = unit === 'day' ? 7 : 1 + let amount = 0 - return
- {dates ? -
onClick(dates.fromDate, dates.toDate)}> - - {value} - - {isEqual ? - {formatDate(dates.fromDate, 'Do MMM YYYY')} - : - - {formatDate(dates.fromDate, 'Do MMM YY')} {formatDate(dates.toDate, 'Do MMM YY')} - } -
: - - No results found - - } -
+ for (let i = 0; i < 200 && candidate.isAfter(today, 'date'); i++) { + candidate = candidate.subtract(step, unit) + amount += step + } + + return { amount, unit } } -export default BankRecDateFilter \ No newline at end of file +/** + * Parse free text into a past date range, or return undefined when it can't be parsed or + * resolves entirely into the future. + */ +const parseDateRange = (value: string): { fromDate: string, toDate: string } | undefined => { + if (!value.trim()) return undefined + + const parsedDate = parse(value, undefined, { forwardDate: false }) + + if (!parsedDate || parsedDate.length === 0) return undefined + + const result = parsedDate[0] + const startKnownValues = knownValuesOf(result.start) + + // Anchor the shift on the start and apply it to both ends, so an explicit range like + // "1st Sept to 30th Sept" keeps its shape instead of having only its end rolled back. + const shift = pastShift(result.start.date(), startKnownValues) + const startDate = dayjs(result.start.date()).subtract(shift.amount, shift.unit).toDate() + const endDate = result.end + ? dayjs(result.end.date()).subtract(shift.amount, shift.unit).toDate() + : undefined + + const today = new Date() + let range: { fromDate: Date, toDate: Date } + + if (endDate) { + const endKnownValues = knownValuesOf(result.end) + // chrono ends "Apr 2025 to Jun 2025" on the 1st of June, but the user means all of it. + const rangeEnd = endKnownValues.month && !endKnownValues.day + ? dayjs(endDate).endOf('month').toDate() + : endDate + range = { fromDate: startDate, toDate: rangeEnd } + } else if (startKnownValues.month && !startKnownValues.day) { + // The user only wants a specific month like "May 2025" - span the whole month + range = { fromDate: dayjs(startDate).startOf('month').toDate(), toDate: dayjs(startDate).endOf('month').toDate() } + } else if (startKnownValues.month && startKnownValues.day && !referentialKeywords.some(keyword => value.toLowerCase().includes(keyword))) { + // If month and day is known, then we should not assume that the user wants to get everything until today + range = { fromDate: startDate, toDate: startDate } + } else { + range = { fromDate: startDate, toDate: today } + } + + // A range that hasn't started yet is never useful for reconciliation. A range that merely + // ends in the future is kept as typed, the same way "This Month" spans the whole month. + if (dayjs(range.fromDate).isAfter(today, 'date')) return undefined + + if (dayjs(range.toDate).isBefore(range.fromDate, 'date')) { + range = { fromDate: range.toDate, toDate: range.fromDate } + } + + return { + fromDate: dayjs(range.fromDate).format(DATE_FORMAT), + toDate: dayjs(range.toDate).format(DATE_FORMAT), + } +} + +export default BankRecDateFilter diff --git a/banking/src/components/features/BankReconciliation/BankReconciliationStatement.tsx b/banking/src/components/features/BankReconciliation/BankReconciliationStatement.tsx index 0815bc8a65e..592acfff844 100644 --- a/banking/src/components/features/BankReconciliation/BankReconciliationStatement.tsx +++ b/banking/src/components/features/BankReconciliation/BankReconciliationStatement.tsx @@ -191,9 +191,9 @@ const BankReconciliationStatementView = () => { const content = _("Below is a list of all entries posted against the bank account {0} which have not been cleared till {1}.", [`${bankAccount?.account}`, `${formatDate(dates.toDate)}`]) - return
+ return
-
+
@@ -201,16 +201,18 @@ const BankReconciliationStatementView = () => { {error && } - {data && } + {data &&
} {data && data.message.result.length > 0 && ( -
-

{_("Bank Reconciliation Statement")}

+
+

{_("Bank Reconciliation Statement")}

row.payment_entry} - maxHeight="min(70vh, 640px)" + className="min-h-0 flex-1" + maxHeight="none" + scrollAreaClassName="flex-1" emptyState={_("No entries with a payment document in this list.")} />
diff --git a/banking/src/components/features/BankReconciliation/BankTransactionList.tsx b/banking/src/components/features/BankReconciliation/BankTransactionList.tsx index 1513e567a4b..a09994bf3e5 100644 --- a/banking/src/components/features/BankReconciliation/BankTransactionList.tsx +++ b/banking/src/components/features/BankReconciliation/BankTransactionList.tsx @@ -245,9 +245,9 @@ const BankTransactionListView = () => { const content = _("Below is a list of all bank transactions imported in the system for the bank account {0} between {1} and {2}.", [`${bankAccount?.account_name}`, `${formattedFromDate}`, `${formattedToDate}`]) - return
+ return
-
+
@@ -278,8 +278,9 @@ const BankTransactionListView = () => { data={filteredResults} columns={transactionColumns} getRowId={(row) => row.name} - maxHeight="calc(100vh - 200px)" - scrollAreaClassName="min-h-[calc(100vh-200px)]" + className="min-h-0 flex-1" + maxHeight="none" + scrollAreaClassName="flex-1" emptyState={ diff --git a/banking/src/components/features/BankReconciliation/IncorrectlyClearedEntries.tsx b/banking/src/components/features/BankReconciliation/IncorrectlyClearedEntries.tsx index fac7e2dc533..2293b41c771 100644 --- a/banking/src/components/features/BankReconciliation/IncorrectlyClearedEntries.tsx +++ b/banking/src/components/features/BankReconciliation/IncorrectlyClearedEntries.tsx @@ -181,9 +181,9 @@ const IncorrectlyClearedEntriesView = () => { const entriesContent = _("Entries below have a posting date after {0} but the clearance date is before {1}.", [`${formattedToDate}`, `${formattedToDate}`]) - return
+ return
-
+

@@ -198,13 +198,15 @@ const IncorrectlyClearedEntriesView = () => { {error && } {data && data.message.result.length > 0 && ( -
-

{_("Incorrectly cleared entries as per the report.")}

+
+

{_("Incorrectly cleared entries as per the report.")}

`${row.payment_entry}-${row.posting_date}`} - maxHeight="min(70vh, 640px)" + className="min-h-0 flex-1" + maxHeight="none" + scrollAreaClassName="flex-1" emptyState={_("No rows to display.")} />
diff --git a/banking/src/components/features/BankReconciliation/MatchAndReconcile.tsx b/banking/src/components/features/BankReconciliation/MatchAndReconcile.tsx index 7549cf74150..dce64e033d7 100644 --- a/banking/src/components/features/BankReconciliation/MatchAndReconcile.tsx +++ b/banking/src/components/features/BankReconciliation/MatchAndReconcile.tsx @@ -37,7 +37,7 @@ import { Link } from "react-router" import { Alert, AlertDescription, AlertTitle } from "@/components/ui/alert" import { InputGroup, InputGroupAddon, InputGroupText } from "@/components/ui/input-group" -const MatchAndReconcile = ({ contentHeight }: { contentHeight: number }) => { +const MatchAndReconcile = () => { const selectedBank = useAtomValue(selectedBankAccountAtom) if (!selectedBank) { @@ -52,15 +52,15 @@ const MatchAndReconcile = ({ contentHeight }: { contentHeight: number }) => { } return <> -
-
-

{_("Unreconciled Transactions")}

- +
+
+

{_("Unreconciled Transactions")}

+
- -
-

{_("Match or Create")}

- + +
+

{_("Match or Create")}

+
@@ -69,16 +69,19 @@ const MatchAndReconcile = ({ contentHeight }: { contentHeight: number }) => { } -/** TanStack requires `estimateSize` for initial scroll range; `measureElement` on each row sets the real height. */ +/** + * TanStack requires `estimateSize` for initial scroll range; `measureElement` on each row sets + * the real height. The scroll container fills its flex parent rather than taking a pixel + * height - the virtualizer observes its own rect, so it stays correct across resizes and any + * layout change above it. + */ function VirtualizedListBody({ items, - height, getItemKey, children, estimateSize = 74, }: { items: T[] - height: number getItemKey: (item: T, index: number) => string | number children: (item: T, index: number) => React.ReactNode estimateSize?: number @@ -100,8 +103,7 @@ function VirtualizedListBody({ return (
({ ) } -const UnreconciledTransactions = ({ contentHeight }: { contentHeight: number }) => { +const UnreconciledTransactions = () => { const bankAccount = useAtomValue(selectedBankAccountAtom) const currency = bankAccount?.account_currency ?? getCompanyCurrency(bankAccount?.company ?? '') @@ -187,14 +189,13 @@ const UnreconciledTransactions = ({ contentHeight }: { contentHeight: number }) } const hasFilters = search !== '' || typeFilter !== 'All' || amountFilter.value !== 0 - const listHeight = contentHeight - 72 if (isLoading) { return } - return
-
+ return
+
@@ -278,7 +279,6 @@ const UnreconciledTransactions = ({ contentHeight }: { contentHeight: number }) transaction.name} > @@ -381,7 +381,7 @@ const UnreconciledTransactionItem = ({ transaction }: { transaction: Unreconcile } -const VouchersSection = ({ contentHeight }: { contentHeight: number }) => { +const VouchersSection = () => { const selectedBank = useAtomValue(selectedBankAccountAtom) const selectedTransactions = useAtomValue(bankRecSelectedTransactionAtom(selectedBank?.name || '')) @@ -402,8 +402,8 @@ const VouchersSection = ({ contentHeight }: { contentHeight: number }) => { return } - return
- + return
+
} @@ -535,11 +535,11 @@ const OptionsForMultipleTransactions = ({ transactions }: { transactions: Unreco } -const OptionsForSingleTransaction = ({ transaction, contentHeight }: { transaction: UnreconciledTransaction, contentHeight: number }) => { +const OptionsForSingleTransaction = ({ transaction }: { transaction: UnreconciledTransaction }) => { const { setTransferModalOpen, setRecordPaymentModalOpen, setRecordJournalEntryModalOpen } = useKeyboardShortcuts() - return
+ return
@@ -602,7 +602,7 @@ const OptionsForSingleTransaction = ({ transaction, contentHeight }: { transacti
{transaction.matched_transaction_rule && } - +
} @@ -774,12 +774,11 @@ const RuleAction = ({ transaction }: { transaction: UnreconciledTransaction }) = ) } -const VouchersForTransaction = ({ transaction, contentHeight }: { transaction: UnreconciledTransaction, contentHeight: number }) => { +const VouchersForTransaction = ({ transaction }: { transaction: UnreconciledTransaction }) => { const { data: vouchers, isLoading, error } = useGetVouchersForTransaction(transaction) const voucherList = vouchers?.message ?? [] - const listHeight = contentHeight - 120 if (error) { return @@ -801,8 +800,8 @@ const VouchersForTransaction = ({ transaction, contentHeight }: { transaction: U
} - return
-
+ return
+
or @@ -818,7 +817,6 @@ const VouchersForTransaction = ({ transaction, contentHeight }: { transaction: U } voucher.name} > diff --git a/banking/src/components/features/BankReconciliation/SelectedTransactionDetails.tsx b/banking/src/components/features/BankReconciliation/SelectedTransactionDetails.tsx index 53ffba910a5..210f2b87e95 100644 --- a/banking/src/components/features/BankReconciliation/SelectedTransactionDetails.tsx +++ b/banking/src/components/features/BankReconciliation/SelectedTransactionDetails.tsx @@ -59,8 +59,8 @@ const SelectedTransactionDetails = ({ transaction, showAccount = false, account
- {transaction.description} - {transaction.reference_number ? {_("Ref")}: {transaction.reference_number} : null} + {transaction.description} + {transaction.reference_number ? {_("Ref")}: {transaction.reference_number} : null} {showAccount && account ? {_("GL Account")}: {account} : null}
diff --git a/banking/src/components/features/BankReconciliation/TransferModalContent.tsx b/banking/src/components/features/BankReconciliation/TransferModalContent.tsx index d24cafebe40..eba905f604b 100644 --- a/banking/src/components/features/BankReconciliation/TransferModalContent.tsx +++ b/banking/src/components/features/BankReconciliation/TransferModalContent.tsx @@ -490,7 +490,7 @@ const RecommendedTransferAccount = ({ transaction, onAccountChange }: { transact {formatDate(data.message.date, 'Do MMM YYYY')}
- {data.message.description} + {data.message.description}
diff --git a/banking/src/components/features/BankStatementImporter/CSV/StatementDetails.tsx b/banking/src/components/features/BankStatementImporter/CSV/StatementDetails.tsx index b8ef25961f5..073645754d1 100644 --- a/banking/src/components/features/BankStatementImporter/CSV/StatementDetails.tsx +++ b/banking/src/components/features/BankStatementImporter/CSV/StatementDetails.tsx @@ -83,10 +83,13 @@ const StatementDetails = ({ data }: Props) => { } + // `progress` is a percentage (drives the bar); `current`/`total` are actual counts. const [progress, setProgress] = useState(0) + const [imported, setImported] = useState({ current: 0, total: 0 }) useFrappeEventListener("bank-rec-statement-import-progress", (event) => { setProgress(event.progress) + setImported({ current: event.current ?? 0, total: event.total ?? 0 }) }) const file_name = data.doc.file.split("/").pop() ?? "" @@ -112,7 +115,9 @@ const StatementDetails = ({ data }: Props) => { {data.doc.status === 'Completed' ? {_("Completed")} : + {loading ? _("Importing...") : data.final_transactions?.length === 1 + ? _("Import 1 transaction") + : _("Import {0} transactions", [data.final_transactions?.length?.toString() || "0"])} }
@@ -129,7 +134,9 @@ const StatementDetails = ({ data }: Props) => {
{progress > 0 &&
- {_("Importing {0} transactions", [progress.toString()])} + {imported.total === 1 + ? _("Importing 1 transaction") + : _("Importing {0} of {1} transactions", [imported.current.toString(), imported.total.toString()])}
} diff --git a/banking/src/components/ui/list-view.tsx b/banking/src/components/ui/list-view.tsx index ddd0c0e7020..2833bf2dff6 100644 --- a/banking/src/components/ui/list-view.tsx +++ b/banking/src/components/ui/list-view.tsx @@ -387,7 +387,7 @@ function ListViewInner({ )} role="columnheader" > -
+
{header.isPlaceholder ? null : flexRender(header.column.columnDef.header, header.getContext())} diff --git a/banking/src/hooks/useFiscalYear.ts b/banking/src/hooks/useFiscalYear.ts index 14a25060ea0..e64950a0b71 100644 --- a/banking/src/hooks/useFiscalYear.ts +++ b/banking/src/hooks/useFiscalYear.ts @@ -1,13 +1,58 @@ import { useFrappeGetCall } from "frappe-react-sdk" +import { useMemo } from "react" +import dayjs from "dayjs" +import { useCurrentCompany } from "./useCurrentCompany" -const useFiscalYear = () => { - - return useFrappeGetCall("erpnext.accounts.utils.get_fiscal_year", undefined, 'fiscal_year', { - revalidateOnFocus: false, - revalidateIfStale: false, - revalidateOnReconnect: false - }) - +export type FiscalYear = { + name: string + year_start_date: string + year_end_date: string } -export default useFiscalYear \ No newline at end of file +/** + * The fiscal year containing today, for the currently selected company. + * + * `company` matters in multi-company setups, where fiscal years can be restricted to + * specific companies. `date` matters because without it `get_fiscal_year` returns the newest + * fiscal year in the system (they're ordered by start date, descending) - which may be one + * created in advance for a year that hasn't started. + */ +const useFiscalYear = () => { + const company = useCurrentCompany() + + const { data, ...rest } = useFrappeGetCall<{ message: FiscalYear | [string, string, string] | false }>( + "erpnext.accounts.utils.get_fiscal_year", + { + date: dayjs().format("YYYY-MM-DD"), + company, + as_dict: 1, + // Return nothing instead of throwing/msgprinting when no fiscal year covers today. + raise_on_missing: 0, + verbose: 0, + }, + company ? `fiscal_year_${company}` : null, + { + revalidateOnFocus: false, + revalidateIfStale: false, + revalidateOnReconnect: false + } + ) + + // get_fiscal_year returns a dict with as_dict, a (name, start, end) tuple without it, and + // false when there's no match - normalise all three. + const fiscalYear = useMemo(() => { + const message = data?.message + if (!message) return undefined + + if (Array.isArray(message)) { + const [name, year_start_date, year_end_date] = message + return { name, year_start_date, year_end_date } + } + + return message + }, [data]) + + return { fiscalYear, ...rest } +} + +export default useFiscalYear diff --git a/banking/src/hooks/useResetScrollOnSearch.ts b/banking/src/hooks/useResetScrollOnSearch.ts new file mode 100644 index 00000000000..8c2c2bb1721 --- /dev/null +++ b/banking/src/hooks/useResetScrollOnSearch.ts @@ -0,0 +1,23 @@ +import { useLayoutEffect, useRef } from "react" + +/** + * Pins a scrollable list back to the top whenever the search term changes. + * + * Dropdowns that do their own filtering (`shouldFilter={false}`) swap a long list for a much + * shorter one while the scroll container keeps its previous offset - which can leave the + * auto-selected first item scrolled out of view. + * + * Returns a ref to attach to the scroll container (e.g. `CommandList`). + */ +const useResetScrollOnSearch = (search: string) => { + const listRef = useRef(null) + + // Layout effect so the reset lands before paint, avoiding a visible jump. + useLayoutEffect(() => { + listRef.current?.scrollTo({ top: 0 }) + }, [search]) + + return listRef +} + +export default useResetScrollOnSearch diff --git a/banking/src/index.css b/banking/src/index.css index 808a76c5efd..f2a02509507 100644 --- a/banking/src/index.css +++ b/banking/src/index.css @@ -1,5 +1,6 @@ @import "tailwindcss"; @import "tw-animate-css"; +@import "./styles/scroll-fade.css"; @font-face { font-family: InterVariable; diff --git a/banking/src/pages/BankReconciliation.tsx b/banking/src/pages/BankReconciliation.tsx index 235c304a4a5..8e5a743088b 100644 --- a/banking/src/pages/BankReconciliation.tsx +++ b/banking/src/pages/BankReconciliation.tsx @@ -1,4 +1,4 @@ -import BankBalance from "@/components/features/BankReconciliation/BankBalance" +import BankAccountBalancePanel from "@/components/features/BankReconciliation/BankBalance" import BankPicker from "@/components/features/BankReconciliation/BankPicker" import BankRecDateFilter from "@/components/features/BankReconciliation/BankRecDateFilter" import BankTransactionUnreconcileModal from "@/components/features/BankReconciliation/BankTransactionUnreconcileModal" @@ -9,10 +9,9 @@ import ActionLog from "@/components/features/ActionLog/ActionLog" import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs" import { TooltipProvider } from "@/components/ui/tooltip" import _ from "@/lib/translate" -import { lazy, Suspense, useLayoutEffect, useRef, useState } from "react" +import { lazy, Suspense } from "react" import { AlertTriangleIcon, CheckCircleIcon, HomeIcon, LandmarkIcon, ListIcon, Loader2Icon, ScrollTextIcon, ShuffleIcon } from "lucide-react" import { Breadcrumb, BreadcrumbItem, BreadcrumbList, BreadcrumbPage, BreadcrumbSeparator } from "@/components/ui/breadcrumb" -import { Badge } from "@/components/ui/badge" import { Empty, EmptyContent, EmptyDescription, EmptyHeader, EmptyMedia, EmptyTitle } from "@/components/ui/empty" import { Button } from "@/components/ui/button" import { useAtomValue } from "jotai" @@ -25,23 +24,13 @@ const IncorrectlyClearedEntries = lazy(() => import('@/components/features/BankR const BankReconciliation = () => { - const [headerHeight, setHeaderHeight] = useState(0) - - const ref = useRef(null) - - useLayoutEffect(() => { - if (ref.current) { - setHeaderHeight(ref.current.clientHeight) - } - }, []) - - const remainingHeightAfterTabs = window.innerHeight - headerHeight - 220 - return (
-
-
-
+ {/* The page owns the viewport height and the tabs/lists below fill what's left, so + the virtualizers size themselves from layout instead of a measured pixel value. */} +
+
+
@@ -54,7 +43,7 @@ const BankReconciliation = () => {
- {_("Banking")} {_("Beta")} + {_("Banking")}
@@ -71,10 +60,8 @@ const BankReconciliation = () => {
- -
- +
@@ -104,42 +91,53 @@ const BankReconciliation = () => { ) } -const BankRecTabs = ({ remainingHeightAfterTabs }: { remainingHeightAfterTabs: number }) => { +const BankRecWorkspace = () => { const selectedBankAccount = useAtomValue(selectedBankAccountAtom) - if (!selectedBankAccount) { - return null - } - - return - - {_("Match and Reconcile")} - {_("Bank Reconciliation Statement")} - {_("Bank Transactions")} - {_("Bank Clearance Summary")} - {_("Incorrectly Cleared Entries")} - - - - - - + return + {/* Picker + tab strip stack on the left, balance panel beside them - the tab strip + fills height the panel needs anyway, so it costs no row of its own. The picker + scrolls horizontally (`min-w-0` lets it shrink so its overflow-x engages) while + the panel stays put, so the figures never scroll away. */} + {/* No gap here: the panel's own `border-s ps-4` supplies the separation, and a gap + would leave dead space the picker's edge fade can't reach. */} +
+
+ + {selectedBankAccount && + {_("Match and Reconcile")} + {_("Reconciliation Statement")} + {_("Transactions")} + {_("Clearance Summary")} + {_("Incorrectly Cleared")} + }
- }> - - + {selectedBankAccount && } +
+ + {selectedBankAccount && <> + + - - - - - - - - - - + + +
+ }> + + + + + + + + + + + + + + } } diff --git a/banking/src/pages/BankStatementImporter.tsx b/banking/src/pages/BankStatementImporter.tsx index 8e6e5345bd7..8a21110e538 100644 --- a/banking/src/pages/BankStatementImporter.tsx +++ b/banking/src/pages/BankStatementImporter.tsx @@ -226,7 +226,7 @@ const StatementImportLog = () => { field: "creation", order: "desc" }, - limit: 10 + limit: 20 }, bankAccount ? undefined : null, { revalidateOnFocus: false }) diff --git a/banking/src/styles/scroll-fade.css b/banking/src/styles/scroll-fade.css new file mode 100644 index 00000000000..2b9a3fde62f --- /dev/null +++ b/banking/src/styles/scroll-fade.css @@ -0,0 +1,94 @@ +/* Scroll-edge fade mask for horizontal scroll containers (the bank picker strip). + Ported from Raven's `scroll-fade-x`; imported by index.css, since Tailwind processes + `@utility` in imported files the same as in the entry file. + + The scroll-timeline keyframes reveal each edge's fade only when there IS content to scroll + in that direction - no fade on the left edge when scrolled fully left, none on the right at + the end. `@property` makes the fade animate smoothly rather than jumping. + + Without scroll-timeline support (Firefox) there is deliberately NO fade at all: the fade + vars stay at their 0px initial value and the gradient stops collapse to the edges. A static + both-edges fallback was tried in Raven and removed - on a container with nothing to scroll + it dimmed the edges anyway, promising content that didn't exist. */ + +@property --scroll-fade-l { + /* length-percentage, NOT length: the fade size is min(12%, …) - a percentage. A + property rejects that value and reverts to initial-value (0px), zeroing the fade. */ + syntax: ""; + inherits: false; + initial-value: 0px; +} + +@property --scroll-fade-r { + syntax: ""; + inherits: false; + initial-value: 0px; +} + +@keyframes scroll-fade-reveal-l { + from { + --scroll-fade-l: 0px; + } + + to { + --scroll-fade-l: var(--_scroll-fade-size-l); + } +} + +@keyframes scroll-fade-reveal-r { + from { + --scroll-fade-r: var(--_scroll-fade-size-r); + } + + to { + --scroll-fade-r: 0px; + } +} + +@utility scroll-fade-x { + --_scroll-fade-size-l: var(--scroll-fade-l-size, + var(--scroll-fade-size, min(12%, calc(var(--spacing, 0.25rem) * 10)))); + --_scroll-fade-size-r: var(--scroll-fade-r-size, + var(--scroll-fade-size, min(12%, calc(var(--spacing, 0.25rem) * 10)))); + /* Eased (smoothstep) alpha ramp, sampled finely so it reads as a smooth curve, NOT fading + all the way to transparent: the edge floors at 0.25 (content dims, never vanishes), ramping + up to a full 1 for the body. The opaque end MUST be 1 or everything would be permanently + dimmed. Stops collapse to the edge when the size animates to 0, so the true first/last card + is never dimmed at rest. Tune the floor - higher (~0.4) = subtler, lower (~0.1) = stronger. */ + --scroll-fade-inline: linear-gradient(to right, + rgba(0, 0, 0, 0.25) 0, + rgba(0, 0, 0, 0.282) calc(var(--scroll-fade-l, 0px) * 0.125), + rgba(0, 0, 0, 0.367) calc(var(--scroll-fade-l, 0px) * 0.25), + rgba(0, 0, 0, 0.487) calc(var(--scroll-fade-l, 0px) * 0.375), + rgba(0, 0, 0, 0.625) calc(var(--scroll-fade-l, 0px) * 0.5), + rgba(0, 0, 0, 0.763) calc(var(--scroll-fade-l, 0px) * 0.625), + rgba(0, 0, 0, 0.883) calc(var(--scroll-fade-l, 0px) * 0.75), + rgba(0, 0, 0, 0.968) calc(var(--scroll-fade-l, 0px) * 0.875), + rgba(0, 0, 0, 1) var(--scroll-fade-l, 0px), + rgba(0, 0, 0, 1) calc(100% - var(--scroll-fade-r, 0px)), + rgba(0, 0, 0, 0.968) calc(100% - var(--scroll-fade-r, 0px) * 0.875), + rgba(0, 0, 0, 0.883) calc(100% - var(--scroll-fade-r, 0px) * 0.75), + rgba(0, 0, 0, 0.763) calc(100% - var(--scroll-fade-r, 0px) * 0.625), + rgba(0, 0, 0, 0.625) calc(100% - var(--scroll-fade-r, 0px) * 0.5), + rgba(0, 0, 0, 0.487) calc(100% - var(--scroll-fade-r, 0px) * 0.375), + rgba(0, 0, 0, 0.367) calc(100% - var(--scroll-fade-r, 0px) * 0.25), + rgba(0, 0, 0, 0.282) calc(100% - var(--scroll-fade-r, 0px) * 0.125), + rgba(0, 0, 0, 0.25) 100%); + -webkit-mask-image: var(--scroll-fade-mask, var(--scroll-fade-inline)); + mask-image: var(--scroll-fade-mask, var(--scroll-fade-inline)); + -webkit-mask-composite: source-in; + mask-composite: intersect; + -webkit-mask-repeat: no-repeat; + mask-repeat: no-repeat; + + @supports (animation-timeline: scroll()) { + animation: + scroll-fade-reveal-l 1ms ease-in-out, + scroll-fade-reveal-r 1ms ease-in-out; + animation-timeline: scroll(self x), scroll(self x); + animation-range: + 0 var(--scroll-fade-reveal, calc(var(--spacing, 0.25rem) * 24)), + calc(100% - var(--scroll-fade-reveal, calc(var(--spacing, 0.25rem) * 24))) 100%; + animation-fill-mode: both; + } +} diff --git a/erpnext/accounts/doctype/bank_statement_import_log/bank_statement_import_log.py b/erpnext/accounts/doctype/bank_statement_import_log/bank_statement_import_log.py index 468bce0e1fd..ee9941ff3eb 100644 --- a/erpnext/accounts/doctype/bank_statement_import_log/bank_statement_import_log.py +++ b/erpnext/accounts/doctype/bank_statement_import_log/bank_statement_import_log.py @@ -375,8 +375,7 @@ class BankStatementImportLog(Document): table["column_mapping"] = guess_column_mapping_by_content(table["rows"]) final_transactions, table["date_format"], table["amount_format"] = build_table_transactions(table) - # Tables with no detectable transactions (ads, summaries, headers) start excluded. - table["included"] = bool(final_transactions) + table["included"] = should_include_table(table, final_transactions) self.pdf_tables = json.dumps(tables) return tables @@ -542,6 +541,8 @@ class BankStatementImportLog(Document): "bank-rec-statement-import-progress", { "progress": round(progress / total_transactions * 100), + "current": progress, + "total": total_transactions, }, doctype="Bank Statement Import Log", docname=self.name, @@ -551,6 +552,7 @@ class BankStatementImportLog(Document): "bank-rec-statement-import-progress", { "progress": 100, + "current": total_transactions, "total": total_transactions, }, doctype="Bank Statement Import Log", @@ -821,6 +823,15 @@ def compute_final_transactions(transaction_rows: list, date_format: str, amount_ """Pure version of the final-transaction builder (date normalized, amount split).""" final_transactions = [] + # Which marker does this statement actually write? A statement that only ever says "Cr" + # is marking the credits as its exceptions, so an unmarked row is a withdrawal; one that + # only ever says "Dr" means the opposite. With both markers present an unmarked row is + # genuinely undetermined, so it stays a withdrawal. + unmarked_is_deposit = False + if amount_format == 'Amount column has "CR"/"DR" values': + markers = {get_amount_cr_dr_marker(row.get("amount")) for row in transaction_rows} + unmarked_is_deposit = markers - {None} == {"dr"} + def parse_amount(transaction_row: dict): if amount_format == "Separate columns for withdrawal and deposit": return get_float_amount(transaction_row.get("withdrawal")), get_float_amount( @@ -829,44 +840,43 @@ def compute_final_transactions(transaction_rows: list, date_format: str, amount_ if amount_format == 'Amount column has "CR"/"DR" values': amount = transaction_row.get("amount") + marker = get_amount_cr_dr_marker(amount) + # The marker carries the direction, so the amount's own sign is ignored. + signed_amount = get_float_amount(amount) or 0 - # If the amount column has CR/DR in it - we should remove any signs (negative or positive) from the amount - float_amount = abs(get_float_amount(amount) or 0) - if "cr" in amount.lower(): - return 0, float_amount - else: - return float_amount, 0 + if marker: + return (0, abs(signed_amount)) if marker == "cr" else (abs(signed_amount), 0) + # An unmarked row takes the opposite direction to the marker this statement + # uses. A negative amount reverses that again (a refund). + is_deposit = unmarked_is_deposit + if signed_amount < 0: + is_deposit = not is_deposit + + return (0, abs(signed_amount)) if is_deposit else (abs(signed_amount), 0) + + # `or 0` below: get_float_amount returns None for an unparseable cell, and a blank + # transaction-type cell comes through as None. Both used to raise. if amount_format == "Amount column has positive/negative values": - amount = get_float_amount(transaction_row.get("amount", "0")) + amount = get_float_amount(transaction_row.get("amount", "0")) or 0 if amount > 0: return 0, abs(amount) else: return abs(amount), 0 + transaction_type = str(transaction_row.get("debit_credit") or "").strip().lower() + amount = abs(get_float_amount(transaction_row.get("amount", "0")) or 0) + if amount_format == 'Transaction type column has "CR"/"DR" values': - transaction_type = transaction_row.get("debit_credit") - amount = get_float_amount(transaction_row.get("amount", "0")) - if "cr" in transaction_type.lower(): - return 0, abs(amount) - else: - return abs(amount), 0 + # "credit" contains "cr". "debit" does not contain "dr", so it correctly falls + # through to the withdrawal side. + return (0, amount) if "cr" in transaction_type else (amount, 0) if amount_format == 'Transaction type column has "C"/"D" values': - transaction_type = transaction_row.get("debit_credit") - amount = get_float_amount(transaction_row.get("amount", "0")) - if transaction_type.lower().strip() == "c": - return 0, abs(amount) - else: - return abs(amount), 0 + return (0, amount) if transaction_type == "c" else (amount, 0) if amount_format == 'Transaction type column has "Deposit"/"Withdrawal" values': - transaction_type = transaction_row.get("debit_credit") - amount = get_float_amount(transaction_row.get("amount", "0")) - if "deposit" in transaction_type.lower(): - return 0, abs(amount) - else: - return abs(amount), 0 + return (0, amount) if "deposit" in transaction_type else (amount, 0) return 0, 0 @@ -910,6 +920,26 @@ def build_table_transactions(table: dict): return final_transactions, date_format, amount_format +def should_include_table(table: dict, final_transactions: list) -> bool: + """ + Whether a freshly extracted PDF table should START as included - only the default state + of the checkbox, which the user can change afterwards. + + It must have yielded transactions, and it must have a Description column mapped. A + transaction table always carries a narration; the summary boxes printed around it - + payment due, credit limit, reward points - are dates and figures only. Otherwise the + HDFC credit-card "Payment Due Date / Total Dues / Minimum Amount Due" box parses as one + transaction and imports a phantom row. + + A description is NOT needed to import (it is not mandatory on Bank Transaction), so a + bank that omits narration still works - its table just starts unticked. + """ + if not final_transactions: + return False + + return any(column.get("maps_to") == "Description" for column in table.get("column_mapping", [])) + + def _clean_cell(cell) -> str: """Normalize a pdfplumber cell: None -> '', collapse wrapped newlines, strip.""" if cell is None: @@ -1055,6 +1085,43 @@ def get_float_amount(amount): return amount +# A "CR"/"DR" marker on the amount itself, at either end: "2,378.00Cr", "Cr 100", +# "INR 50.90 Cr.", "DR 1,234.50". +# `(?![a-zA-Z])` rather than `\b` on the leading form: there is no word boundary between +# the "r" of "Cr100" and the digit, but there IS one inside "CREDIT" and "DRAFT". +AMOUNT_CR_DR_PATTERN = re.compile(r"^\s*(cr|dr)(?![a-zA-Z])\.?|(?:^|[\s\d.)])(cr|dr)\b\.?\s*$", re.IGNORECASE) + + +def get_amount_cr_dr_marker(amount) -> str | None: + """ + Return "cr" or "dr" if the amount cell carries a direction marker of its own, else None. + + What is left after removing the marker has to look like an amount - it must hold a digit + and at most a short currency token - so that text which merely starts or ends with the + letters is not read as a marker. That guard is what separates "Cr 100" from a + description that bled into the amount column, like "Dr Smith Clinic 500". + """ + if not isinstance(amount, str): + return None + + match = AMOUNT_CR_DR_PATTERN.search(amount) + if not match: + return None + + # Only the marker itself is removed - the surrounding character the pattern needed to + # anchor on (a digit, say) stays part of the remainder. + group = 1 if match.group(1) else 2 + start, end = match.span(group) + remainder = amount[:start] + amount[end:] + + if not any(char.isdigit() for char in remainder): + return None + if sum(char.isalpha() for char in remainder) > 3: + return None + + return match.group(group).lower() + + def get_file_properties(transactions: list): """ From the transaction rows, try to figure out the following: @@ -1075,6 +1142,8 @@ def get_file_properties(transactions: list): 'Transaction type column has "C"/"D" values': 0, } + amount_column_has_cr_dr = False + for transaction in transactions: date_format = transaction.get("date_format") @@ -1092,33 +1161,40 @@ def get_file_properties(transactions: list): if not amount: continue - if isinstance(amount, str) and ("cr" in amount.lower() or "dr" in amount.lower()): + debit_credit = str(transaction.get("debit_credit") or "").strip().lower() + + # One vote per row, most specific signal first. Order matters: "withdrawal" contains + # "dr", so it must be matched before the loose cr/dr check or a Deposit/Withdrawal + # column reads as CR/DR. "debit" needs listing because, unlike "credit", it does not + # contain "dr". The final else means every row votes, even an unrecognised type. + if get_amount_cr_dr_marker(amount): + amount_column_has_cr_dr = True amount_format_frequency['Amount column has "CR"/"DR" values'] += 1 - - # Check if there's a debit_credit column containing "cr"/"dr" - if transaction.get("debit_credit", None): - if ( - "cr" in transaction.get("debit_credit", "").lower() - or "dr" in transaction.get("debit_credit", "").lower() - ): - amount_format_frequency['Transaction type column has "CR"/"DR" values'] += 1 - elif ( - "deposit" in transaction.get("debit_credit", "").lower() - or "withdrawal" in transaction.get("debit_credit", "").lower() - ): - amount_format_frequency['Transaction type column has "Deposit"/"Withdrawal" values'] += 1 - elif (transaction.get("debit_credit", "").lower().strip() == "c") or ( - transaction.get("debit_credit", "").lower().strip() == "d" - ): - amount_format_frequency['Transaction type column has "C"/"D" values'] += 1 - - # Else assume that the amount is expressed as positive/negative value + elif "deposit" in debit_credit or "withdrawal" in debit_credit: + amount_format_frequency['Transaction type column has "Deposit"/"Withdrawal" values'] += 1 + elif debit_credit in ("c", "d"): + amount_format_frequency['Transaction type column has "C"/"D" values'] += 1 + elif any(token in debit_credit for token in ("cr", "dr", "debit")): + amount_format_frequency['Transaction type column has "CR"/"DR" values'] += 1 else: + # Nothing said which direction this is, so assume the amount carries the sign. amount_format_frequency["Amount column has positive/negative values"] += 1 most_common_date_format = max(date_format_frequency, key=date_format_frequency.get) most_common_amount_format = max(amount_format_frequency, key=amount_format_frequency.get) + # With no votes at all (no rows, or every amount blank) max() would return whichever key + # happens to be first in the dict. Say what we mean instead. + if not amount_format_frequency[most_common_amount_format]: + most_common_amount_format = "Amount column has positive/negative values" + + # A CR/DR amount column is proved by a single marker, not by a majority: both formats + # describe the same column, and an unmarked row is only the default direction, not + # evidence against the notation. Statements mark just the exceptions - one HDFC + # credit-card page has 18 rows and a single "50.90Cr". + if amount_column_has_cr_dr and most_common_amount_format == "Amount column has positive/negative values": + most_common_amount_format = 'Amount column has "CR"/"DR" values' + return most_common_date_format, most_common_amount_format diff --git a/erpnext/accounts/doctype/bank_statement_import_log/test_bank_statement_import_log.py b/erpnext/accounts/doctype/bank_statement_import_log/test_bank_statement_import_log.py index 5d2c02ec305..6caca5441f2 100644 --- a/erpnext/accounts/doctype/bank_statement_import_log/test_bank_statement_import_log.py +++ b/erpnext/accounts/doctype/bank_statement_import_log/test_bank_statement_import_log.py @@ -11,12 +11,14 @@ from erpnext.accounts.doctype.bank_statement_import_log.bank_statement_import_lo detect_column_mapping, detect_header_row, extract_pdf_tables, + get_amount_cr_dr_marker, get_float_amount, get_statement_details, guess_column_mapping_by_content, reextract_pdf_table, set_header_index, set_pdf_table_header, + should_include_table, update_column_mapping, update_pdf_tables, ) @@ -124,6 +126,184 @@ class TestBankStatementImportLog(ERPNextTestSuite, AccountsTestMixin): self.assertIsNone(get_float_amount("ABCD")) self.assertIsNone(get_float_amount("****")) + # ------------------------------------------------------------------ # + # Amount format detection + # ------------------------------------------------------------------ # + + def test_amount_cr_dr_marker(self): + """The marker is read at either end of the cell, but only next to the amount.""" + for amount in ("2,378.00Cr", "50.90 CR", "INR 50.90 Cr.", "1000cr", "5cr", "(100) Cr"): + self.assertEqual(get_amount_cr_dr_marker(amount), "cr", amount) + + for amount in ("2,378.00Dr", "50.90 DR", "1000dr", "-100 Dr"): + self.assertEqual(get_amount_cr_dr_marker(amount), "dr", amount) + + # Some banks put the marker in front of the digits instead. + for amount in ("Cr 100", "Cr100", "CR INR 100", "cr 0.00"): + self.assertEqual(get_amount_cr_dr_marker(amount), "cr", amount) + + for amount in ("Dr 100", "Dr100", "Dr. 1,234.50"): + self.assertEqual(get_amount_cr_dr_marker(amount), "dr", amount) + + for amount in ("100.00", "-2,000.00", "INR 25,236.00", "", None, 100.0): + self.assertIsNone(get_amount_cr_dr_marker(amount), amount) + + # Text that merely starts or ends with the letters must not be read as a marker, or + # a description that bled into the amount column would reclassify the statement. + for amount in ( + "CREDIT CARD PAYMENT 500", + "DRAFT 100", + "Dr Smith Clinic 500", + "DR AMBEDKAR ROAD BRANCH 500", + "500 CRC", + "Cheque Dr", + "Cr", + ): + self.assertIsNone(get_amount_cr_dr_marker(amount), amount) + + def test_sparsely_marked_cr_dr_amount_column(self): + """One marker is enough to prove a CR/DR amount column - it is not a majority vote. + + A real HDFC credit-card page carries 18 rows and a single "50.90Cr": the unmarked + rows are ordinary purchases, and only the exceptions are marked. A frequency vote + therefore picked "positive/negative" 17-1 and imported that lone credit as a debit. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Transaction Description", "Amount (in Rs.)"], + ["21/07/2026", "ITC MAURYA NEW DELHI", "2,495.00"], + ["22/07/2026", "ZOMATO LIMITED Gurugram", "1,288.68"], + ["23/07/2026", "SWIGGY Bangalore", "532.00"], + ["26/07/2026", "SWIGGY Bangalore", "1,043.00"], + ["27/07/2026", "PETRO SURCHARGE WAIVER", "50.90Cr"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Amount column has "CR"/"DR" values') + # Only "Cr" appears, so it is the marked exception and unmarked rows are debits. + self.assertEqual(doc.total_credits, 50.90) + self.assertEqual(doc.total_credit_transactions, 1) + self.assertEqual(doc.total_debits, 5358.68) + self.assertEqual(doc.total_debit_transactions, 4) + + def test_dr_only_statement_treats_unmarked_rows_as_deposits(self): + """The mirror image of a Cr-only statement: only withdrawals are marked. + + The unmarked default cannot be hardcoded to the debit, because which side gets + marked varies by bank. It is derived from the markers the statement actually uses - + here only "Dr" appears, so "Dr" is the exception and everything unmarked is a + deposit. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Amount"], + ["01/04/2026", "ATM WITHDRAWAL", "2,000.00Dr"], + ["03/04/2026", "SALARY", "20,000.00"], + ["05/04/2026", "INTEREST", "150.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Amount column has "CR"/"DR" values') + self.assertEqual(doc.total_debits, 2000.0) + self.assertEqual(doc.total_debit_transactions, 1) + self.assertEqual(doc.total_credits, 20150.0) + self.assertEqual(doc.total_credit_transactions, 2) + + def test_leading_cr_dr_markers(self): + """Some banks print the marker in front of the amount.""" + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Amount"], + ["01/04/2026", "ATM WITHDRAWAL", "Dr 2,000.00"], + ["03/04/2026", "SALARY", "Cr 20,000.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Amount column has "CR"/"DR" values') + self.assertEqual(doc.total_debits, 2000.0) + self.assertEqual(doc.total_credits, 20000.0) + + def test_partially_marked_cr_dr_amount_column(self): + """A CR/DR amount column stays CR/DR even when some rows carry no marker. + + Every unmarked row used to also vote for "positive/negative", so an ordinary + statement with a few unmarked rows was detected as positive/negative and a + "2000.00Dr" was then imported as a deposit. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Amount", "Balance"], + ["01/04/2026", "OPENING FEE", "100.00", "9,900.00"], + ["03/04/2026", "SALARY", "20000.00Cr", "29,900.00"], + ["05/04/2026", "ATM WDL", "2000.00Dr", "27,900.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Amount column has "CR"/"DR" values') + # Both markers appear, so an unmarked row is undetermined and stays a debit. + self.assertEqual(doc.total_debits, 2100.0) + self.assertEqual(doc.total_debit_transactions, 2) + self.assertEqual(doc.total_credits, 20000.0) + self.assertEqual(doc.total_credit_transactions, 1) + + def test_deposit_withdrawal_type_column(self): + """The word Withdrawal contains "dr", so a loose CR/DR check claims this column first. + + It then reads "Deposit" (which has no "cr" in it) as a withdrawal, flipping the + direction of every credit in the statement. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Transaction Type", "Amount"], + ["01/04/2026", "ATM WDL", "Withdrawal", "2,000.00"], + ["03/04/2026", "SALARY", "Deposit", "20,000.00"], + ["05/04/2026", "ATM WDL", "Withdrawal", "500.00"], + ] + ) + + self.assertEqual( + doc.detected_amount_format, 'Transaction type column has "Deposit"/"Withdrawal" values' + ) + self.assertEqual(doc.total_debits, 2500.0) + self.assertEqual(doc.total_debit_transactions, 2) + self.assertEqual(doc.total_credits, 20000.0) + self.assertEqual(doc.total_credit_transactions, 1) + + def test_unrecognised_type_column_falls_back_to_signed_amount(self): + """An unrecognised transaction type must not stop the amount being read. + + No tally was incremented for these rows, so max() returned the first key - + "Separate columns for withdrawal and deposit" - and, with no such columns in the + file, every amount came through as None. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Transaction Type", "Amount"], + ["01/04/2026", "ATM WDL", "NEFT", "-2,000.00"], + ["03/04/2026", "SALARY", "IMPS", "20,000.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, "Amount column has positive/negative values") + self.assertEqual(doc.total_debits, 2000.0) + self.assertEqual(doc.total_credits, 20000.0) + + def test_blank_transaction_type_cell(self): + """A blank type cell used to raise - `None.lower()` - instead of parsing the row.""" + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Transaction Type", "Amount"], + ["01/04/2026", "ATM WDL", "Dr", "2,000.00"], + ["03/04/2026", "SALARY", "Cr", "20,000.00"], + ["05/04/2026", "UNKNOWN", None, "500.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Transaction type column has "CR"/"DR" values') + # The unmarked row has no direction of its own, so it counts as a withdrawal. + self.assertEqual(doc.total_debits, 2500.0) + self.assertEqual(doc.total_credits, 20000.0) + # ------------------------------------------------------------------ # # PDF statement import # ------------------------------------------------------------------ # @@ -159,7 +339,8 @@ class TestBankStatementImportLog(ERPNextTestSuite, AccountsTestMixin): else: table["header_index"] = None table["column_mapping"] = guess_column_mapping_by_content(table["rows"]) - table["included"] = True + final_transactions, _df, _af = build_table_transactions(table) + table["included"] = should_include_table(table, final_transactions) return table def test_pdf_multi_page_kept_separate_and_unioned(self): @@ -197,6 +378,74 @@ class TestBankStatementImportLog(ERPNextTestSuite, AccountsTestMixin): final, _df, _af = build_table_transactions(ad_table) self.assertEqual(final, []) + def test_pdf_summary_box_not_auto_included(self): + """A summary box that happens to parse as one transaction must not start included. + + The "Payment Due Date / Total Dues / Minimum Amount Due" block on an HDFC + credit-card statement has a date column and a figures column, so it yields a single + transaction - the due date and the minimum amount - and used to import as a phantom + row. What it does not have, and a real transaction table always does, is a narration. + """ + summary_box = { + "header_index": 1, + "rows": [ + ["Statement Date:17/08/2025", "Card No: 4341 55XX XXXX 2754", ""], + ["Payment Due Date", "Total Dues", "Minimum Amount Due"], + ["06/09/2025", "73,200.00", "3,660.00"], + ["Credit Limit", "Available Credit Limit", "Available Cash Limit"], + ["", "32,800", ""], + ], + "column_mapping": [ + {"index": 0, "header_text": "Payment Due Date", "variable": "a", "maps_to": "Date"}, + {"index": 1, "header_text": "Total Dues", "variable": "b", "maps_to": "Do not import"}, + {"index": 2, "header_text": "Minimum Amount Due", "variable": "c", "maps_to": "Amount"}, + ], + } + + final, _df, _af = build_table_transactions(summary_box) + # It really does parse as a transaction - that is why the previous check missed it. + self.assertEqual(len(final), 1) + self.assertFalse(should_include_table(summary_box, final)) + + # The transaction table beside it, which does carry a narration, still starts included. + transactions = self._auto_map( + { + "rows": [ + ["Date", "Transaction Description", "Amount (in Rs.)"], + ["21/07/2025", "ITC MAURYA NEW DELHI", "2,495.00"], + ["27/07/2025", "PETRO SURCHARGE WAIVER", "50.90Cr"], + ] + } + ) + self.assertTrue(transactions["included"]) + + def test_pdf_table_without_description_still_importable(self): + """No narration column means "starts unticked", NOT "cannot be imported". + + `description` is not mandatory on Bank Transaction, so a bank that omits narration + must still import once the user ticks the table. + """ + table = { + "header_index": 0, + "rows": [ + ["Date", "Amount", "Balance"], + ["01/04/2025", "500.00", "9,500.00"], + ["03/04/2025", "20000.00", "29,500.00"], + ], + "column_mapping": [ + {"index": 0, "header_text": "Date", "variable": "a", "maps_to": "Date"}, + {"index": 1, "header_text": "Amount", "variable": "b", "maps_to": "Amount"}, + {"index": 2, "header_text": "Balance", "variable": "c", "maps_to": "Balance"}, + ], + } + + final, _df, _af = build_table_transactions(table) + self.assertFalse(should_include_table(table, final)) + + # The transactions themselves are intact and importable. + self.assertEqual(len(final), 2) + self.assertEqual([t["date"] for t in final], ["2025-04-01", "2025-04-03"]) + def test_headerless_content_mapping(self): """Without a header row, columns are guessed from their contents.""" rows = [