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

[17.0][OU-ADD] sale: Migration to 17.0 #4561

Closed
wants to merge 1 commit into from

Conversation

acpMicrocom
Copy link
Contributor

No description provided.

@legalsylvain
Copy link
Contributor

legalsylvain commented Sep 19, 2024

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Sep 19, 2024
@pedrobaeza
Copy link
Member

For each line (or batch of lines) of the analysis file where there's a nothing to do, you have to indicate why is nothing to do. It seems a bit weird that there's nothing to do on the DB schema part.

@rvalyi

This comment was marked as off-topic.

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Indeed, as said by pedro, please analyse the difference between V16 and V17.

I took a quick look. For exemple, new 'locked' and removal of 'done' state should be handled.

Thanks !

@acpMicrocom

This comment was marked as off-topic.

@legalsylvain

This comment was marked as off-topic.

@rvalyi

This comment was marked as off-topic.

@legalsylvain
Copy link
Contributor

Hi @rvalyi. Thanks for the explanation. I allowed me to put all the odoo-module-diff comment in off-topic, as the current PR is about "sale" migration. Feel free to open dedicated issue if you want to talk about the akretion tool. thanks !

Comment on lines 16 to 33
def _sale_sale_order(env):
openupgrade.logged_query(
env.cr,
"""
ALTER TABLE sale_order
ADD COLUMN IF NOT EXISTS locked BOOLEAN,
ADD COLUMN IF NOT EXISTS temp_state VARCHAR
""",
)
openupgrade.logged_query(
env.cr,
"""
UPDATE sale_order
SET temp_state = state
""",
)
openupgrade.logged_query(
env.cr,
"""
UPDATE sale_order
SET state = 'sale'
WHERE state = 'done'
""",
)


Copy link
Contributor

Choose a reason for hiding this comment

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

Hi. Thanks for your patch !

question: why create a temporary field. I mean, you can simply add locked field, update the value (based on state = "done"), then update state = sale, where state = done). ?

convention: could change the name of the function, for a more explicit one. "def _sale_sale_order" is not very explicit. something like "def _sale_order_populate_locked_field" looks better. (or something like that.)

thanks !

@acpMicrocom
Copy link
Contributor Author

During the execution of the tests in the pull request for the sale module migration script, the following error occurs:

ERROR: column res_partner.complete_name does not exist

This error occurs because the complete_name field of the res_partner model is not available until the base addon is installed or updated. To resolve the problem, it is necessary to make sure that the base module is installed and updated before running the tests.

@dansanti
Copy link

dansanti commented Nov 8, 2024

Hello!

I think with this query, we can merge it with website_sale_digital

openupgrade.logged_query(
    env.cr,
    """
    UPDATE product_document
    SET attached_on = 'sale_order'
    FROM ir_attachment ir
    WHERE product_document.ir_attachment_id = ir.id
      AND ir.product_downloadable = TRUE
    """
)```

)
openupgrade.delete_sql_constraint_safely(
env, "sale", "sale_order", "sale_order_date_order_conditional_required"
)
Copy link
Member

Choose a reason for hiding this comment

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

the ORM does this automatically

SET locked = True, state = 'sale'
WHERE state = 'done'
""",
)
Copy link
Member

Choose a reason for hiding this comment

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

locking should be conditional on the creating user having the sale.group_auto_done_setting group.

Better do this in post-migration anyways, then you don't need to create the column manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @hbrunn,

Thank you for your comment. You can proceed with the changes in this Pull Request, as we currently don’t have any hours allocated for contributions

@hbrunn
Copy link
Member

hbrunn commented Dec 9, 2024

@acpMicrocom done in #4691

@dansanti I think this belongs into the website_sale migration, I left a comment there mentioning your input

@hbrunn hbrunn closed this Dec 9, 2024
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.

7 participants