Skip to content
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

[15.0][MIG] base_import_async #434

Open
wants to merge 2 commits into
base: 15.0
Choose a base branch
from

Conversation

Rad0van
Copy link

@Rad0van Rad0van commented May 25, 2022

I am building sort of a ETL for migrating/importing lots of data into Odoo 15. Asynchronous import is part of the big picture. To my big surprise I found out the modules are present in 15.0 branch but not installable and not working at all. As I need this functionality right now I have attempted to fix this module and improve it a little bit.

The changes include:

Didn't bother with test_base_import_async as that's broken in a different way: it relies on account data not being present in vanilla installation since version 13.0 I guess (account journals with codes CABA, 121000, 400000).

@Rad0van Rad0van force-pushed the fix_imp_base_import_async branch from 0150b98 to 9376b49 Compare May 25, 2022 20:49
@Rad0van Rad0van force-pushed the fix_imp_base_import_async branch from 24791f3 to 2d9260a Compare May 26, 2022 07:24
@simahawk
Copy link
Contributor

there's already a PR here #430

@simahawk simahawk changed the title [15.0][FIX][IMP]base_import_async [15.0][MIG] base_import_async May 31, 2022
@simahawk
Copy link
Contributor

Didn't bother with test_base_import_async as that's broken in a different way: it relies on account data not being present in vanilla installation since version 13.0 I guess (account journals with codes CABA, 121000, 400000).

If you want to move on w/ this PR you must migrate test_base_import_async too.

@Rad0van
Copy link
Author

Rad0van commented May 31, 2022

there's already a PR here #430

How on Earth did I miss that? Sorry, my bad.

@Rad0van
Copy link
Author

Rad0van commented May 31, 2022

Didn't bother with test_base_import_async as that's broken in a different way: it relies on account data not being present in vanilla installation since version 13.0 I guess (account journals with codes CABA, 121000, 400000).

If you want to move on w/ this PR you must migrate test_base_import_async too.

Well now this is an interesting situation. In 14.0 test_base_import_async is present and marked as installable. But it fails the same way as in 15.0:

2022-05-31 09:32:48,416 166658 ERROR 14CE-main unittest.suite: ERROR: setUpClass (odoo.addons.test_base_import_async.tests.test_base_import_async.TestBaseImportAsync)
Traceback (most recent call last):
  File "/mnt/slow/rex/Programming/odoo/odoo-14.0/odoo/tests/common.py", line 173, in _handleClassSetUp
    setUpClass()
  File "/mnt/slow/rex/Programming/odoo/odoo-14.0/.addons-local/test_base_import_async/tests/test_base_import_async.py", line 60, in setUpClass
    [("code", "=", "400000")]
  File "/mnt/slow/rex/Programming/odoo/odoo-14.0/odoo/addons/base/models/ir_model.py", line 2032, in _update_xmlids
    rows.add((prefix, suffix, record._name, record.id, noupdate))
  File "/mnt/slow/rex/Programming/odoo/odoo-14.0/odoo/fields.py", line 3821, in __get__
    raise ValueError("Expected singleton: %s" % record)
ValueError: Expected singleton: account.journal(5, 13)
 
2022-05-31 09:32:48,642 166658 INFO 14CE-main odoo.modules.loading: Module test_base_import_async loaded in 0.72s (incl. 0.13s test), 28 queries (+7 test) 
2022-05-31 09:32:48,642 166658 ERROR 14CE-main odoo.modules.loading: Module test_base_import_async: 0 failures, 1 errors of 0 tests 

The reason being this:

cls.env["ir.model.data"]._update_xmlids(
[
{
"xml_id": "test_base_import_async.testjournal_xmlid",
"record": cls.env["account.journal"].search(
[("code", "=", "CABA")]
),
},
{
"xml_id": "test_base_import_async.a_recv_xmlid",
"record": cls.env["account.account"].search(
[("code", "=", "121000")]
),
},
{
"xml_id": "test_base_import_async.a_sale_xmlid",
"record": cls.env["account.account"].search(
[("code", "=", "400000")]
),
},
]
)

There are no such account.journal or account.account records to be found in account module nor in the whole 14.0 codebase. That's why I skipped an attempt to migrate this into 15.0

@simahawk any idea what's going on here?

@github-actions
Copy link

github-actions bot commented Oct 2, 2022

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 2, 2022
@github-actions github-actions bot closed this Nov 6, 2022
@Rad0van
Copy link
Author

Rad0van commented Nov 7, 2022

@simahawk would you please re-open this one? One day I'll get to migrate that test case module as well and this one is working. Thank you.

@rousseldenis
Copy link
Contributor

@simahawk @OCA/core-maintainers Could you reopen this ? Thanks

@hbrunn hbrunn reopened this Mar 13, 2023
@hbrunn hbrunn added no stale Use this label to prevent the automated stale action from closing this PR/Issue. and removed stale PR/Issue without recent activity, it'll be soon closed automatically. labels Mar 13, 2023
@BT-jcolmeiro
Copy link

BT-jcolmeiro commented Feb 20, 2024

Hi @simahawk @gurneyalex recently merged migration of this module to v16.0(#523 ) that was based on this PR and #485 now closed due inactivity, without migration of test_base_import_async. Is this requirement noi longer needed? how can we proceed so there is no module version gap? FYI @dzungtran89

@simahawk
Copy link
Contributor

@BT-jcolmeiro thanks for the ping. It seems I missed a bunch of notifications. since a while.. Sorry guys 🙏

I'd say we can move on here by starting from scratch from #485 (just reset hard on that branch) to preserve the same history that we have in 16.0.

There are no such account.journal or account.account records to be found in account module nor in the whole 14.0 codebase. That's why I skipped an attempt to migrate this into 15.0

Regarding the test module: That's weird. In fact I don't see the tests of that module running on 14.0. WTH?

For the short term: as this work is pending since quite some time I'd move on w/o test module, especially IF you had the chance to test it in real life and can honestly say "It's fully working".

For the long term, I would consider getting rid of the module and rewrite the tests w/o dependency on account and test the export in the base module by using a base model or a custom one (w/ odoo-test-helper) . To be added to the ROADMAP and maybe w/ a GH issue to not forget about that.

My $0.02.

@sbidoul as you are the author of the test module: WDYT?

@Rad0van
Copy link
Author

Rad0van commented Feb 20, 2024

@simahawk / @hbrunn / @rousseldenis can you maybe reopen this one? If the 16.0 version got accepted without the test module, maybe we can finish this one as well. Will have a look at the test module and see if I'm able to re-create it as part of this one.

@hbrunn
Copy link
Member

hbrunn commented Feb 20, 2024

it is still open

@Rad0van
Copy link
Author

Rad0van commented Feb 20, 2024

it is still open

I must be going blind ;-)

@kanda999
Copy link

Functional review: It works as expected in my local env.

Copy link

@kanda999 kanda999 left a comment

Choose a reason for hiding this comment

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

I have confirmed that this module works and imports excel files with queue every 100 recoerds. 16.0 version is also merged without testing so I approve this.

In the long term, tests should be added to keep the quality the code in good condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no stale Use this label to prevent the automated stale action from closing this PR/Issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants