[12.0][FIX] base: don't create user_admin if already exists

[MiquelRForgeFlow]

Fixes #1766

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

[pedrobaeza]

Use ON CONFLICT DO NOTHING instead.

[MiquelRForgeFlow]

Done

[pedrobaeza]

But the question in the issue remains: if this is not added by core, if we do this, the record can point to other thing that it's not even the same model.

[StefanRijnhart]

Agreed. Would be very interesting to know how this xml entry ended up there. Was it an earlier, failed run of the pre migration script? (but the rest of the script does not look reentrant either)

[pedrobaeza]

@mreficent can you answer?

[MiquelRForgeFlow]

No idea. @yann-papouin or @kenji-autotest should answer haha
But my two cents are YES, it seems a a rerun of a failed pre-migration.

[pedrobaeza]

OK, then unless we demonstrate this is a real issue, this doesn't serve, and starting to put safeguards everywhere is not the best option IMO.

[pedrobaeza]

I believe this happened due to #1879, so closing for now while not proofs about it.

[MiquelRForgeFlow]

I happened to meet a database that already have an user_admin. Thus, I need this fix.

[pedrobaeza]

@mreficent I don't think so. Maybe there was an error in the middle of your migration and thus end-migration scripts weren't executed.

[MiquelRForgeFlow]

No. I am sure. The xmlid base.user_admin existed in an old odoo version. I will find it.

[pedrobaeza]

Unless you prove it pointing to the core code that makes this, for me it's 👎

[MiquelRForgeFlow]

Found 🤣 🎉. Around between v5 and v6, I think, not sure.
Creation commit of user_admin: odoo/odoo@3e1b346
Deletion commit of user_admin: odoo/odoo@5886791

Also, I will show you my proof:

So I will merge this PR!

[pedrobaeza]

This is the result of not cleaning your DB with database_cleanup, not a normal process IMO. You should care about your data status that way. And you are not sure about where that record points to. And what about if other third party module adds that record and you are overwriting it for other purpose?

Let's hear @StefanRijnhart's opinion about this, but any way, if merged:

• A comment must specify why we are using that.
• res_id correctness must be assured: ON CONFLICT DO UPDATE... seems better.

[MiquelRForgeFlow]

I decided to use the "ON CONSTRAINT" mode to be more specific and avoid unexpected cases and be safer 😉

[pedrobaeza]

My concern about overwriting an existing third party XML-ID still exists... Why don't you simply remove that XML-ID in your specific database.

[MiquelRForgeFlow]

Third party XML-ID will not be affected because they will have a different module, not the "base" module.

[pedrobaeza]

You can declare that XML-ID from another module, but OK, I won't say more objections. Waiting @StefanRijnhart's approval.

[StefanRijnhart]

Interestingly the tests say
2019-07-05 17:44:16,220 6685 ERROR openupgrade OpenUpgrade: base: error in migration script base/migrations/12.0.1.3/pre-migration.py: constraint "ir_model_data_module_name_uniq" for table "ir_model_data" does not exist. Any ideas?

[MiquelRForgeFlow]

I searched a bit. This constraint existed in v8 but was removed in v9. In my force-push, I just put it because I was just having in front of me a database with that constraint and I thought it was a normal constraint. Shit.

[MiquelRForgeFlow]

Well, I proceeded finally with the most safely way possible hahaha

Please, update review

[StefanRijnhart]

Alright!

[pedrobaeza]

I still miss the explanation in a comment in the code of why doing all of this.

[MiquelRForgeFlow]

/ocabot merge

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Rebased to 12.0-ocabot-merge-pr-1779-by-mreficent-bump-no, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at d2c1810. Thanks a lot for contributing to OCA. ❤️

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 12.0.