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

[16.0][MIG] shopinvader_search_engine: Migration to 16.0 + REFACTORING #1390

Conversation

qgroulard
Copy link
Contributor

@qgroulard qgroulard commented Aug 7, 2023

No description provided.

@qgroulard qgroulard force-pushed the 16.0-mig-shopinvader_search_engine branch from 6fa829e to 264511f Compare August 21, 2023 14:04
@qgroulard qgroulard force-pushed the 16.0-mig-shopinvader_search_engine branch 3 times, most recently from b92f3c9 to 9a7444e Compare August 30, 2023 09:11
@sebastienbeau
Copy link
Contributor

@qgroulard can you check the lint and the reason of the failling test.

It will be great to merge this PR, as it introduce shopinvader_product.

Thanks

@qgroulard
Copy link
Contributor Author

@qgroulard can you check the lint and the reason of the failling test.

It will be great to merge this PR, as it introduce shopinvader_product.

Thanks

I still have a bit of work here. Few open points will be discussed with lmignon today.

I'll keep you posted and make this PR ready asap.

@qgroulard qgroulard force-pushed the 16.0-mig-shopinvader_search_engine branch 2 times, most recently from 9680e9c to c0b6a35 Compare September 7, 2023 09:17
@qgroulard

This comment was marked as resolved.

Copy link
Contributor

@hparfr hparfr 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 this work @qgroulard

In order to keep the discussion in one place, let continue on #1386, specifically about:

  1. the product/category link with shopinvader_backend
  2. if _compute_main_product deserve it's own module (in oca)

)
return res

def _get_price(
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure it's still needed ?

there was some changes between v14 and v16 in product regarding the price calculation

Copy link
Contributor

Choose a reason for hiding this comment

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

shopinvader_product_description/models/product_template.py Outdated Show resolved Hide resolved
@qgroulard qgroulard force-pushed the 16.0-mig-shopinvader_search_engine branch from c0b6a35 to 7f0e3f4 Compare September 8, 2023 10:16
@qgroulard

This comment was marked as resolved.

shopinvader_product/schemas/product.py Outdated Show resolved Hide resolved
shopinvader_product/schemas/product.py Outdated Show resolved Hide resolved
@qgroulard
Copy link
Contributor Author

seo_title used to be computed based on the backend's website name.
What should we do now ?

def _build_seo_title(self):
"""
Build the SEO product name
:return: str
"""
self.ensure_one()
return "{} | {}".format(
self.name or "", self.backend_id.website_public_name or ""
)

@lmignon
Copy link
Collaborator

lmignon commented Sep 18, 2023

seo_title used to be computed based on the backend's website name. What should we do now ?

def _build_seo_title(self):
"""
Build the SEO product name
:return: str
"""
self.ensure_one()
return "{} | {}".format(
self.name or "", self.backend_id.website_public_name or ""
)

🤔 🤔 🤔 🤔 🤔 🤔 🤔

What about adding a new seo_title_postfix field on the search_engine backend? (translatable=True?)

@sebastienbeau @hparfr

@hparfr
Copy link
Contributor

hparfr commented Sep 18, 2023

What about adding a new seo_title_postfix field on the search_engine backend? (translatable=True?)

@sebastienbeau @hparfr

In which module ?

@qgroulard qgroulard force-pushed the 16.0-mig-shopinvader_search_engine branch 4 times, most recently from 3470811 to 3078ced Compare September 19, 2023 15:33
@hparfr
Copy link
Contributor

hparfr commented Sep 19, 2023

hi

Have you seen this pr https://github.com/OCA/sale-channel/pull/7/files#diff-aa8725b71193423d895954631ef05a6d125aae74feb2242645d1c4ae4357a676R20 ?

It already contain all the needed logic to select products and categories for se export, it also implement levels and filters categories.

For working with shopinvader, it needs a base module for the creation of an ir_export and a is a serializer_type (previously it was the shopinvader module, it may be replaced by a bare minimum shopinvader_product module
and a glue module for adding ir_export_lines.

You can checkout this branch, it's based on restapi: https://github.com/akretion/odoo-shopinvader/tree/16.0-shopinvader-sale-channel

@qgroulard
Copy link
Contributor Author

hi

Have you seen this pr https://github.com/OCA/sale-channel/pull/7/files#diff-aa8725b71193423d895954631ef05a6d125aae74feb2242645d1c4ae4357a676R20 ?

It already contain all the needed logic to select products and categories for se export, it also implement levels and filters categories.

For working with shopinvader, it needs a base module for the creation of an ir_export and a is a serializer_type (previously it was the shopinvader module, it may be replaced by a bare minimum shopinvader_product module and a glue module for adding ir_export_lines.

You can checkout this branch, it's based on restapi: https://github.com/akretion/odoo-shopinvader/tree/16.0-shopinvader-sale-channel

Hi,
I don't think we need to depend on sale_channel to compute parent and children categories.
IMO these fields belong here, whatever source able to give the index (e.g. the sale_channel) can pass it in the context and the fields will be computed accordingly.

@sebastienbeau sebastienbeau added this to the 16.0 milestone Sep 25, 2023
@qgroulard qgroulard force-pushed the 16.0-mig-shopinvader_search_engine branch 2 times, most recently from bd16806 to e25ff71 Compare September 28, 2023 15:10
@lmignon lmignon force-pushed the 16.0-mig-shopinvader_search_engine branch from 0f6c402 to 909474a Compare October 13, 2023 06:38
@lmignon lmignon changed the title WIP: [16.0][MIG] shopinvader_search_engine: Migration to 16.0 + REFACTORING [16.0][MIG] shopinvader_search_engine: Migration to 16.0 + REFACTORING Oct 13, 2023
@lmignon
Copy link
Collaborator

lmignon commented Oct 13, 2023

/ocabot merge nobump

4 similar comments
@lmignon
Copy link
Collaborator

lmignon commented Oct 13, 2023

/ocabot merge nobump

@lmignon
Copy link
Collaborator

lmignon commented Oct 13, 2023

/ocabot merge nobump

@lmignon
Copy link
Collaborator

lmignon commented Oct 13, 2023

/ocabot merge nobump

@hparfr
Copy link
Contributor

hparfr commented Oct 13, 2023

/ocabot merge nobump

@hparfr
Copy link
Contributor

hparfr commented Oct 13, 2023

looks like the bot is sleeping.

btw, some of my remarks are not adressed; such as the get_price()

@hparfr hparfr self-requested a review October 13, 2023 07:35
@lmignon
Copy link
Collaborator

lmignon commented Oct 13, 2023

looks like the bot is sleeping.

btw, some of my remarks are not adressed; such as the get_price()

The price is in the roadmap. It will be taken into account in an other PR. We need to move forward since we've tons of PR linked to each others and it's now difficult to have a clear view of all the work already done.

@shopinvader-git-bot
Copy link

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1390-by-lmignon-bump-nobump, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Oct 13, 2023
Signed-off-by lmignon
@shopinvader-git-bot
Copy link

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-1390-by-lmignon-bump-nobump, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Oct 13, 2023
Signed-off-by lmignon
@shopinvader-git-bot
Copy link

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-1390-by-lmignon-bump-nobump, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Oct 13, 2023
Signed-off-by lmignon
@shopinvader-git-bot
Copy link

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1390-by-lmignon-bump-nobump, awaiting test results.

shopinvader-git-bot pushed a commit that referenced this pull request Oct 13, 2023
Signed-off-by lmignon
@shopinvader-git-bot
Copy link

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1390-by-hparfr-bump-nobump, awaiting test results.

@shopinvader-git-bot
Copy link

@lmignon your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1390-by-lmignon-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@shopinvader-git-bot
Copy link

@hparfr your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1390-by-hparfr-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@shopinvader-git-bot shopinvader-git-bot merged commit d97dd6a into shopinvader:16.0 Oct 13, 2023
3 checks passed
@lmignon lmignon deleted the 16.0-mig-shopinvader_search_engine branch October 13, 2023 10:01
@shopinvader-git-bot
Copy link

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

@qgroulard
Copy link
Contributor Author

What about adding a new seo_title_postfix field on the search_engine backend? (translatable=True?)
@sebastienbeau @hparfr

In which module ?

I've opened this PR: #1434

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.

6 participants