Conversation
bosd
force-pushed
the
17.0-imp-l10nl_bank-script
branch
from
35ce9b5 to
ebc7d3b
Compare
hbrunn
left a comment
hbrunn
left a comment
There was a problem hiding this comment.
we'll need a migration script here that renames the xmlid of adyen and possibly other ones where the xmlid is changed. and then force-loads the file to update names like sns.
if you can easily instruct your statistical parrot to rewrite the script to use xlrd and requests instead of the pretty heavy pandas dependency, it would be nice because the former are already dependencies of odoo. but i won't block on this one
| processed_banks.append((record_id, name, bic)) | ||
|
|
||
| logging.info("✅ Data processed successfully.") | ||
| return sorted(processed_banks, key=lambda x: x[1]) |
There was a problem hiding this comment.
| return sorted(processed_banks, key=lambda x: x[1]) | |
| return sorted(processed_banks, key=lambda x: x[0]) |
to keep sorting by bic as the existing file does, should give a smaller diff and keep ordering stable for name changes
bosd
force-pushed
the
17.0-imp-l10nl_bank-script
branch
from
ebc7d3b to
67fe689
Compare
|
@hbrunn Thanks for the review. |
hbrunn
left a comment
hbrunn
left a comment
There was a problem hiding this comment.
nearly every line of the migration scripts is wrong, but they won't run anyways because the version number isn't increased in the manifest
please only submit code you tested and verified to work as expected
| """ | ||
| This post-migration script re-enables the noupdate="1" flag on the bank | ||
| data file to prevent accidental data overwrites during future module upgrades. | ||
| """ |
There was a problem hiding this comment.
the comments in this file are very misleading, as they don't match what the code actually does
| This post-migration script re-enables the noupdate="1" flag on the bank | ||
| data file to prevent accidental data overwrites during future module upgrades. | ||
| """ | ||
| _logger.info("Starting post-migration: Restoring noupdate flag.") |
There was a problem hiding this comment.
it's quite uncommon to be that verbose, but then it also doesn't really hurt
| data file to prevent accidental data overwrites during future module upgrades. | ||
| """ | ||
| _logger.info("Starting post-migration: Restoring noupdate flag.") | ||
| api.Environment(cr, SUPERUSER_ID, {}) |
| _logger.info( | ||
| "Post-migration successful: Restored noupdate='1' for %s.", xml_file_path | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
generally, if some step of a migration script fails, the whole thing should fail, so the exception handling shouldn't be there at all.
if this code ever ran it would have also hidden all the things wrong with it
|
|
||
| # Define the XML file to be reloaded. This is the same file that was temporarily | ||
| # changed in the pre-migration step. | ||
| xml_file_path = "l10n_nl_bank/data/res_bank_data.xml" |
There was a problem hiding this comment.
it's wrong to pass the path of the module
| # Get the module name. This is crucial for a multi-module environment. | ||
| try: | ||
| module = env.ref("l10n_nl_bank").module | ||
| except ValueError: | ||
| _logger.error( | ||
| "Could not determine module name. Please check if 'l10n_nl_bank' exists." | ||
| ) | ||
| return |
There was a problem hiding this comment.
this whole section is nonsense and causes the script to exit early
| """ | ||
| This pre-migration script updates the XML ID for the Adyen bank | ||
| and temporarily sets noupdate="0" on the bank data file | ||
| to allow for a full data reload during the module upgrade. | ||
| """ |
| mode="update", | ||
| noupdate=True, | ||
| kind="data", | ||
| ) |
There was a problem hiding this comment.
this call does not actually cause data to be overwritten, and fails anyways
3 participants
As mentioned in #424