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][ADD] sale_seasonality #3264

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

kevinkhao
Copy link
Contributor

@kevinkhao kevinkhao commented Aug 8, 2024

@kevinkhao kevinkhao marked this pull request as draft August 8, 2024 07:40
@kevinkhao kevinkhao force-pushed the 16.0-sale_campaign_seasonality branch 3 times, most recently from 8ce1417 to ceec141 Compare August 8, 2024 13:33
@kevinkhao kevinkhao changed the title [16.0][ADD] sale_campaign_seasonality [16.0][ADD] sale_seasonality Aug 8, 2024
@kevinkhao kevinkhao force-pushed the 16.0-sale_campaign_seasonality branch 5 times, most recently from a3886c3 to 650054d Compare August 12, 2024 13:00
@kevinkhao kevinkhao marked this pull request as ready for review August 12, 2024 13:14
@mathieudelva
Copy link

Hello @kevinkhao, shouldn't the campaign field be made required on sales orders? To see orders on sale report
Otherwise, look good for me.

@kevinkhao kevinkhao force-pushed the 16.0-sale_campaign_seasonality branch from 650054d to 138d5d5 Compare August 27, 2024 06:48
@kevinkhao
Copy link
Contributor Author

kevinkhao commented Aug 27, 2024

Hello @kevinkhao, shouldn't the campaign field be made required on sales orders? To see orders on sale report Otherwise, look good for me.

@mathieudelva this would make sense for the usage of this module. However, adding a required field for an optional functionality seems heavy-handed to me. On our side, we used sale_exception to force our client to use campaigns on sale orders

@kevinkhao kevinkhao force-pushed the 16.0-sale_campaign_seasonality branch from 138d5d5 to 1ce7578 Compare September 2, 2024 20:32
@rousseldenis
Copy link
Sponsor Contributor

@kevinkhao So, to be coherent, shouldn't it be named sale_simple_seasonality ?

@bealdav
Copy link
Member

bealdav commented Sep 18, 2024

@rousseldenis thanks for your suggestion but I think a better plan could be to converge in future version, i.e. 18.0 to product_seasonnality (product_simple_seasonnality in 16.0) and sale_seasonnality.
But we can't do it for now because of the previous module.

Maybe @camptocamp could plug its features in product_simple_seasonnality 16.0 / product_seasonnality 18.0

@kevinkhao
Copy link
Contributor Author

@kevinkhao So, to be coherent, shouldn't it be named sale_simple_seasonality ?

@rousseldenis as of now the difference is minimal but it makes sense to revisit and create a discussion around these functionalities and C2C's modules when migration time comes

Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

Some remarks.
Also could be nice to have minimal demo data to match campaign with seasons to see data in 'Report' pivot table ?
Maybe you could add a screenshot with these data to make this module more attractive ?

EDIT: I suppose than in Sale 'Report' pivot table, date in Y axis should be replaced by Campaign first. Currently you defined only season not campaign it seems. @PaulGoubert could you confirm ?

image

Thanks a lot

# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).

{
"name": "Product Campaign Seasonality",
Copy link
Member

Choose a reason for hiding this comment

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

to fix, same as technical name I suppose

{
"name": "Product Campaign Seasonality",
"summary": """
Product campaign seasonality""",
Copy link
Member

Choose a reason for hiding this comment

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

to fix

"license": "AGPL-3",
"author": "Akretion,Odoo Community Association (OCA)",
"website": "https://github.com/OCA/sale-workflow",
"depends": ["product_simple_seasonality", "sale"],
Copy link
Member

Choose a reason for hiding this comment

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

missing utm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sale depends on utm

Copy link
Member

Choose a reason for hiding this comment

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

you're right

@kevinkhao
Copy link
Contributor Author

kevinkhao commented Sep 19, 2024

EDIT: I suppose than in Sale 'Report' pivot table, date in Y axis should be replaced by Campaign first.

@bealdav I think the module should preserve Odoo's native functionality as much as possible and putting seasonality as 1st filter should be done in custom

@bealdav
Copy link
Member

bealdav commented Sep 19, 2024

Thanks a lot fir these improvements, but i think this picture represent more you dev. Don't you think ?
image

@kevinkhao
Copy link
Contributor Author

Thanks a lot fir these improvements, but i think this picture represent more you dev. Don't you think ? image

done

This module modifies campaigns to allow for tracking seasonality and campaigns of sales for better reporting.


.. image:: /sale_product_set/static/description/sale_order.png
Copy link
Member

Choose a reason for hiding this comment

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

Where is this image ?

Copy link
Member

Choose a reason for hiding this comment

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

This syntax is better I suppose

.. figure:: ../static/description/my.png

"views/utm_campaign.xml",
"security/campaign_seasonality.xml",
"reports/sale_report.xml",
"demo/data.xml",
Copy link
Member

Choose a reason for hiding this comment

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

Please demo data should not be in production, thanks

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.

5 participants