Skip to content

10.0 website sale - #968

Merged
pedrobaeza merged 4 commits into
OCA:10.0from
gfcapalbo:10.0-website_sale
Jul 28, 2017
Merged

10.0 website sale#968
pedrobaeza merged 4 commits into
OCA:10.0from
gfcapalbo:10.0-website_sale

Conversation

@gfcapalbo

Copy link
Copy Markdown

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

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

@pedrobaeza

Copy link
Copy Markdown
Member

Please start the PR with a clean commit history (not 7 commits)

@gfcapalbo
gfcapalbo force-pushed the 10.0-website_sale branch 2 times, most recently from 7d2dd49 to 10e3a3f Compare July 20, 2017 10:16

@MiquelRForgeFlow MiquelRForgeFlow 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.

in the product.pricelist table, you should fill the field website_id by self.env['website'].search([], limit=1) as default in the cases where it is empty

also please check travis, I dunno why it fails

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.

# now that all the pricelist_ids have their corresponding website_id and selectable
or something similar

@pedrobaeza

Copy link
Copy Markdown
Member

Where is the analysis work file?

@MiquelRForgeFlow

Copy link
Copy Markdown
Contributor

Why my commit of sale_crm is here? 😵

@gfcapalbo

Copy link
Copy Markdown
Author

@mreficent rebase error, fixing

@gfcapalbo
gfcapalbo force-pushed the 10.0-website_sale branch 5 times, most recently from 475b5df to 71ec287 Compare July 26, 2017 11:08
@gfcapalbo

Copy link
Copy Markdown
Author

rebased, added defaults, linted, passing.

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.

pl_model.search([('id', 'not in', pricelists)]) is simpler

@gfcapalbo
gfcapalbo force-pushed the 10.0-website_sale branch from 80fbc24 to 0241bc1 Compare July 26, 2017 15:06
@gfcapalbo

Copy link
Copy Markdown
Author

improved pricelist scanning and rebased again

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still misses the analysis work file.

pricelist.write({'website_id': pricelist._default_website().id})
# now that all pricelists have a their corresponding website_id
# and selectable drop support table
sql = "drop table website_pricelist_openupgrade_10"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't drop this table, as it can be used for other modules that inherit old website.pricelist model and needs to make a migration.

sql = """select pricelist_id, website_id, selectable from
website_pricelist_openupgrade_10"""
cr.execute(sql)
pricelists = []

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pricelist_ids please

cr.execute(sql)
pricelists = []
for pricelist_id, website_id, selectable in cr.fetchall():
pl_model.search([('id', '=', pricelist_id)]).write({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't make a search, but a browse.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm thinking that this is better done with an update query:

UPDATE product_pricelist pp
SET website_id = wp.id,
    selectable = wp.selectable
FROM website_pricelist_openupgrade_10 wp
WHERE wp.pricelist_id = pp.id

(and make it with openupgrade.logged_query)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

agreed , query, logged.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this query does not consider those pricelists that need to be set at the current default website, so anyways we are just copying the old column so we can save the first loop with the same query , but we still need the second to update all those possible pricelist ids not in the website_pricelist_openupgrade_10 column with the default website. addressed in 20c76159af016f73e7b271727ae9bba0cfb962ba waiting for checks to pass.

for pricelist_id, website_id, selectable in cr.fetchall():
pl_model.search([('id', '=', pricelist_id)]).write({
'website_id': website_id,
'selectable': selectable

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

end comma for reducing possible future diffs.

@gfcapalbo

Copy link
Copy Markdown
Author

@pedrobaeza used your query for the mig of the non-default pricelists.
also added some explanation on the analysis file

legacy column pricelists and assign default website to them
@gfcapalbo
gfcapalbo force-pushed the 10.0-website_sale branch from cd6645b to 7ae4728 Compare July 27, 2017 11:27
FROM website_pricelist_openupgrade_10 wp
WHERE wp.pricelist_id = pp.id
"""
cr.execute(sql)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not done with openupgrade.logged_query.

cr.execute(sql)
pricelist_ids = cr.fetchall()
for pricelist in pl_model.search([('id', 'not in', pricelist_ids)]):
pricelist.write({'website_id': pricelist._default_website().id})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cache the default website in a previous variable for avoiding successive calls to the method.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I did this in case the default calculation became more complex in a near future (e.g. per-pricelists default websites) , as things stand, you are right, but wouldn't you agree to sacrifice performance to increase modularity in a one-time run like a migration?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK

@pedrobaeza

Copy link
Copy Markdown
Member

Hey, I'm seeing a problem with Travis:

IntegrityError: insert or update on table "product_pricelist" violates foreign key constraint "product_pricelist_website_id_fkey"

DETAIL:  Key (website_id)=(3) is not present in table "website".

@gfcapalbo
gfcapalbo force-pushed the 10.0-website_sale branch from 5046b27 to d5a2cd6 Compare July 28, 2017 08:35
@MiquelRForgeFlow

Copy link
Copy Markdown
Contributor

ready to be merged 👍

@pedrobaeza
pedrobaeza merged commit 1a297fa into OCA:10.0 Jul 28, 2017
@gfcapalbo
gfcapalbo deleted the 10.0-website_sale branch July 28, 2017 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants