Skip to content

[19.0][FIX] account_statement_import_sheet_file: Remaining fixes on top of #958 refactoring - #969

Open
cav-adhoc wants to merge 1 commit into
OCA:19.0from
adhoc-dev:19.0-fix-remaining-issues
Open

[19.0][FIX] account_statement_import_sheet_file: Remaining fixes on top of #958 refactoring#969
cav-adhoc wants to merge 1 commit into
OCA:19.0from
adhoc-dev:19.0-fix-remaining-issues

Conversation

@cav-adhoc

@cav-adhoc cav-adhoc commented Jun 23, 2026

Copy link
Copy Markdown

This PR applies the remaining fixes from the closed PR #957 that were not covered by the refactoring PR #958 (which separated XLS/XLSX into standalone modules).

Changes

1. account_statement_import_sheet_mapping.py

Remove duplicate amount_column field

The field was defined twice — the first definition was dead code that never took effect because Python class resolution uses the last definition. Removed to avoid confusion and noise.

Fix typo: "igned" → "signed" in amount_type help

The help text read "use igned amount" which is clearly a typo for "use signed amount".

Add _clear_amount_columns onchange

When the user changes amount_type, the previously configured column fields (amount_column, debit_credit_column, amount_debit_column, amount_credit_column) become semantically invalid. This onchange clears them automatically, preventing stale/invalid configurations from being submitted.

Improve header_lines_skip_count string/help

Renamed string from "Header lines skip count" to "Header row number" with help "Row number where the column headers are located (first row is 0)." The old name implied a skip count when it's actually a 0-based row index — the field value IS the row number, not how many rows to skip.

2. account_statement_import_sheet_parser.py

Case-insensitive column name matching in _get_column_indexes

Different banks export files with varying capitalization (e.g., "Date" vs "date" vs "DATE"). The old code only did exact match via header.index(), which would raise ValueError on case mismatch. Now tries exact match first, then falls back to case-insensitive matching before raising. This method is inherited by both the XLS and XLSX parsers, so all formats benefit.

str() cast in _parse_decimal

The XLS parser (_get_xls_row_values) does not stringify cell values, so raw int/float values from xlrd can reach _parse_decimal via _get_values_from_column. Without this cast, re.sub(r"[^\d...]+", "", value) crashes with a TypeError. The XLSX parser already stringifies values, so this is mainly a safety net for XLS and any future parser.

3. i18n/es.po

Updated translations for the new header_lines_skip_count string and help text.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hi @alexey-pelykh,
some modules you are maintaining are being modified, check this out!

@OCA-git-bot OCA-git-bot added mod:account_statement_import_sheet_file Module account_statement_import_sheet_file series:19.0 labels Jun 23, 2026
@pedrobaeza pedrobaeza added this to the 19.0 milestone Jun 23, 2026
@pedrobaeza pedrobaeza changed the title [FIX] account_statement_import_sheet_file: Remaining fixes on top of #958 refactoring [19.0][FIX] account_statement_import_sheet_file: Remaining fixes on top of #958 refactoring Jun 23, 2026
@cav-adhoc
cav-adhoc force-pushed the 19.0-fix-remaining-issues branch from 011cc1f to fb18373 Compare June 23, 2026 14:33
@pedrobaeza

Copy link
Copy Markdown
Member

Hi, Camilla, thanks for the head-up.

Please check pre-commit, and write here the changes done and the motivation.

@alexey-pelykh alexey-pelykh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning these up. One small, non-blocking robustness note on the new case-insensitive fallback in _get_column_indexes:

if h.lower() == column_name_or_index.lower()

column_name_or_index is safe (it comes from the Char mapping field, so it's always a string by the time it reaches here), but h is a raw header cell. If a header cell arrives as a non-string — which can happen with spreadsheet sources where a header is numeric- or date-typed — then h.lower() raises AttributeError instead of the clean ValueError the exact-match path used to raise. This PR already guards for exactly this shape in _parse_decimal (if not isinstance(value, str): value = str(value)), so str(h).lower() here would keep the two consistent.

Co-Reviewed-By: Claude Opus 4.8 noreply@anthropic.com

 refactoring

- Remove duplicate amount_column field definition
- Fix typo 'igned' -> 'signed' in amount_type help text
- Add _clear_amount_columns onchange to clear irrelevant fields on type change
- Improve header_lines_skip_count string/help (row number, not skip count)
- Add case-insensitive column name matching in _get_column_indexes
- Add str() cast in _parse_decimal for non-string cell values
- Update es.po translations for changed strings
@cav-adhoc
cav-adhoc force-pushed the 19.0-fix-remaining-issues branch from fb18373 to 72c8ca9 Compare July 8, 2026 13:37
@cav-adhoc

Copy link
Copy Markdown
Author

Thanks for cleaning these up. One small, non-blocking robustness note on the new case-insensitive fallback in _get_column_indexes:

if h.lower() == column_name_or_index.lower()

column_name_or_index is safe (it comes from the Char mapping field, so it's always a string by the time it reaches here), but h is a raw header cell. If a header cell arrives as a non-string — which can happen with spreadsheet sources where a header is numeric- or date-typed — then h.lower() raises AttributeError instead of the clean ValueError the exact-match path used to raise. This PR already guards for exactly this shape in _parse_decimal (if not isinstance(value, str): value = str(value)), so str(h).lower() here would keep the two consistent.

Co-Reviewed-By: Claude Opus 4.8 noreply@anthropic.com

@alexey-pelykh Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mod:account_statement_import_sheet_file Module account_statement_import_sheet_file series:19.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants