[18.0][MIG] website_sale_affiliate migration from 16.0 to 18.0 - #1228
[18.0][MIG] website_sale_affiliate migration from 16.0 to 18.0#1228jans23 wants to merge 7 commits into
Conversation
08325cd to
1f5d6ec
Compare
|
@fcayre please review. |
|
@pilarvargas-tecnativa could you review this please? |
fe0825f to
3c41315
Compare
pilarvargas-tecnativa
left a comment
There was a problem hiding this comment.
Hello,
Thank you for your contribution. Please check the commit history as it is incomplete.
Regards!
26aed71 to
cb183c3
Compare
@pilarvargas-tecnativa thank you for the review. Could you please check if the current PR version has been prepared as expected? Thanks! |
|
@pilarvargas-tecnativa Updated accordingly. Please check. |
| "valid_hours": 24, | ||
| "valid_sales": 1, | ||
| } | ||
| ) |
There was a problem hiding this comment.
This change doesn't make any difference either
There was a problem hiding this comment.
Caused by pre-commit, which is recommended during module migration according to OCA
There was a problem hiding this comment.
Moved to a separate commit from migration commit.
| affiliate = request.env["sale.affiliate"].sudo().find_from_kwargs(**kwargs) | ||
| if not affiliate: | ||
| return | ||
|
|
There was a problem hiding this comment.
It is best to avoid leaving spaces between lines of code
There was a problem hiding this comment.
In this case I'd argue that this change can stay since we're separating different logical blocks (line 16 is a return statement).
cb183c3 to
e357fc7
Compare
|
@pilarvargas-tecnativa I've replied to your comments, could you please have a look. Thank you! |
e357fc7 to
c2c9f95
Compare
* [ADD] website_sale_affiliate: Create module * Add sale.affiliate and sale.affiliate.request models * Override WebsiteSale controller shop method to store affiliate id & key kwargs of shop urls in session * Override sale order create method to store affiliate request * Add affiliate and request views, links in Sales/Affiliates * Add affiliate_request_id to sale.order form view * Add request sequence record, make default for new affiliates * Add access rights to sales users/managers * Add tests * Add readme instructions * [IMP] website_sale_affiliate: Make requested changes * Update readme * Add controller override to catch product urls * Remove redundant return None * Replace if statements with try/except when appropriate * Change api decorator on sale.affiliate.find_from_session method from model -> model_cr_context * Add check to avoid unnecessary db search in find_from_session if no affiliate_id * Fix tests * Add new tests * [IMP] website_sale_affiliate: Make requested changes * Un-nest try/except statements for readability * Rename kwargs: ref, key -> aff_ref, aff_key * Improve request search speed for large recordsets * Document similarly-named methods for clarity * Tweak tests as needed * Add additional test * Add roadmap to readme * [REM] website_sale_affiliate: Remove affiliate request IP-matching * [IMP] website_sale_affiliate: Change affiliate request creation * Create affiliate requests regardless of whether a sale occurs * Add basic conversion/sales stats to affiliate model and views * Update tests * Add demo data * Clean up views * Remove line used for debugging * Add field string for clarity * [FIX] website_sale_affiliate: Fix compatibility issues, tests * Fix controller method overrides to avoid conflicts * Match assertAlmostEqual decimal places to amount used in fields * Remove/replace tests with unneeded url_open calls * Rename files, variables for clarity * Clean up demo/test data * [IMP] website_sale_affiliate: Clarify affiliate request name * Add help to name field on affiliate request model
* Handle the case of no web session, which will raise a `RuntimeError('object unbound')` when attempting to access it. This happens during XML record creations, but likely other places such as in the shell
1e2c380 to
7e7ae03
Compare
|
@pilarvargas-tecnativa I've made changes as per your suggestions, could you please have another look? Thanks! |
|
@pilarvargas-tecnativa just a friendly reminder for you. Thanks! |
| ) | ||
|
|
||
| @api.model | ||
| @api.model_create_multi |
There was a problem hiding this comment.
Is this change to @api.model_create_multi really needed for the migration?
As it is now, the method is decorated as multi-create but still receives vals as a single dict and processes order as a single record. So it looks half-adapted and could be misleading or problematic if several records are created at once.
If there is no specific need to support multi-create here, I think it would be cleaner to keep the previous @api.model signature for the migration.
| except KeyError: | ||
| pass | ||
| except KeyError as err: | ||
| _logger.debug(err) |
There was a problem hiding this comment.
Is there any reason to log this exception?
Previously, the absence of aff_ref was silently ignored, which seems to be the expected and common case. I'm wondering whether this debug log adds any value, or if it would be better to keep the previous behavior.
| except (AttributeError, ValueError): | ||
| pass | ||
| except (AttributeError, ValueError) as err: | ||
| _logger.debug("Could not store affiliate info: %s", err) |
There was a problem hiding this comment.
Same question here: is there any specific reason for adding this log?
Since this is a migration, I'd prefer to avoid introducing new logging unless it addresses a known issue or provides information that is actually useful in production.
7e7ae03 to
bf5a716
Compare
|
@pilarvargas-tecnativa Thank you very much for the review. I just addressed it in the code. |
b3a6958 to
77951fd
Compare
|
@pedrobaeza this got two reviews and it could be merged I think. |
|
But there aren't approvals. |
|
@pilarvargas-tecnativa All your comments are addressed. Please have another look. |
You don’t need a separate commit to migrate the module as a FIX for the migration, simply include the changes in the migration commit |
77951fd to
f254502
Compare
|
@pilarvargas-tecnativa Squased as requested. |
No description provided.