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

[ADD] support update mode where we never try to create records #80

Merged
merged 1 commit into from
Oct 6, 2017

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Oct 3, 2017

this is meant to allow loading noupdate="1" records and being sure nothing bad happens when doing this.

Currently, if you load_data with records that were noupdate, forcecreate=False and deleted, odoo will generate a create, which fails if there are any required fields that are not included in the xml

cc @pedrobaeza

@StefanRijnhart
Copy link
Member

Can you introduce a new value for mode, so that no confusion arises with the original modes from the Odoo method (I'm thinking init_no_create). In fact, I am already confused. What happens if you want to update a record that has the noupdate flag in ir_model_data? It looks like the record does not get updated. Maybe you need to pass mode='init' to the upstream method.

Can you move that phrase 'will not try to create a record with partial data in case the record was removed in the database.' to the upper method docstring, so that it shows up in the documentation?

@hbrunn
Copy link
Member Author

hbrunn commented Oct 5, 2017

if you update a record with the noupdate flag, it depends on the mode what happens: nothing in mode=update, an update in init. Or a create if the record doesn't exist, and that's what breaks here.

Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM 👍

elif mode == 'init_no_create':
for fp2 in _get_existing_records(cr, fp, module_name):
tools.convert_xml_import(
cr, module_name, fp2, idref, mode=mode
Copy link
Member

Choose a reason for hiding this comment

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

Change this to init. Please rebase also on latest branch for seeing Travis result.

Copy link
Member Author

Choose a reason for hiding this comment

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

good one. done

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@StefanRijnhart
Copy link
Member

CI is still broken. I'm restarting Travis.

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Looks like adding lxml to requirements.txt might help fix Travis.

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Thanks, now green! @pedrobaeza are you happy as well?

@pedrobaeza
Copy link
Member

Yes, sorry, I'm in the travel back with limited access. Holger, squash at your taste and please merge.

@StefanRijnhart StefanRijnhart merged commit 6b3d130 into OCA:master Oct 6, 2017
@StefanRijnhart
Copy link
Member

Thanks @pedrobaeza, have a good journey!

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

Successfully merging this pull request may close these issues.

4 participants