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.

Sign up for GitHub

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

Jump to bottom

[17.0][MIG] stock_change_qty_reason #2030

Merged
merged 37 commits into from
Sep 18, 2024
Merged

[17.0][MIG] stock_change_qty_reason #2030

merged 37 commits into from
Sep 18, 2024

Conversation

JasminSForgeFlow
Copy link
Contributor

Standard Migration

@ForgeFlow

@rousseldenis
Copy link
Contributor

/ocabot migration stock_change_qty_reason

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone May 14, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request May 14, 2024
Open
50 tasks
LoisRForgeFlow
LoisRForgeFlow reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Functional review. Preset reasons work well, the free text reasons are not usable though. With preset reasons disabled, you cannot edit the reason field while doing the inv. adjustment.

Also, not sure if both reasons fields should be displayed when using preset reasons.

image

@JasminSForgeFlow JasminSForgeFlow mentioned this pull request May 14, 2024
Open
@JasminSForgeFlow
Copy link
Contributor Author

Functional review. Preset reasons work well, the free text reasons are not usable though. With preset reasons disabled, you cannot edit the reason field while doing the inv. adjustment.

Also, not sure if both reasons fields should be displayed when using preset reasons.

image

The first reason is free text when we select any reason (2nd reason) in line while doing adjustment and click on apply, it will copy reason (2nd field) value to reason (1st field), and reason (2nd field) value set as False. reason(1st field) value is for historical purpose as reason (2nd filed) will be set False when adjustment done

@LoisRForgeFlow
Copy link
Contributor

Functional review. Preset reasons work well, the free text reasons are not usable though. With preset reasons disabled, you cannot edit the reason field while doing the inv. adjustment.
Also, not sure if both reasons fields should be displayed when using preset reasons.
image

The first reason is free text when we select any reason (2nd reason) in line while doing adjustment and click on apply, it will copy reason (2nd field) value to reason (1st field), and reason (2nd field) value set as False. reason(1st field) value is for historical purpose as reason (2nd filed) will be set False when adjustment done

@JasminSForgeFlow Sorry for the late reply.

If you want to keep it make it at least optional=hide, but in general it doesn't look very clean to me to keep both fields displayed. You should either go for preset reasons or not, if you starte with free text, you could do some custom migration or a custom module to show both reasons. In other words, we should think more on clean installations than on past sins or legacy issues.

Also, you haven't answered my second comment: the free text reason code is not editable even without pre-set reasons enabled.

rousseldenis and others added 21 commits September 17, 2024 16:45
@rousseldenis @JasminSForgeFlow
Add new module to manage change product quantity reason
2a2ae63
@oca-transbot @JasminSForgeFlow
OCA Transbot updated translations from Transifex
bd001a9
@GillesTephaneMeyomesse @JasminSForgeFlow
[MIG] stock_change_qty_reason
4ef13b9
@oca-transbot @JasminSForgeFlow
OCA Transbot updated translations from Transifex
448078e
@JasminSForgeFlow
[MIG] stock_change_qty_reason: Migration to 11.0
aa2f618
@HviorForgeFlow @JasminSForgeFlow
[11.0][IMP] Add optional encoded reason management
2a85a21
@HviorForgeFlow @JasminSForgeFlow
[MIG] stock_change_qty_reason: Migration to 12.0
7f83906
@HviorForgeFlow @JasminSForgeFlow
[12.0][IMP] Explode reason from Inventory Adjustment to lines
69ac566
@OCA-git-bot @JasminSForgeFlow
[UPD] README.rst
3c94918
@OCA-git-bot @JasminSForgeFlow
[ADD] icon.png
5cded95
@OCA-git-bot @JasminSForgeFlow
[UPD] README.rst
048a250
@gurneyalex @JasminSForgeFlow
Translated using Weblate (French)
e221683 
Currently translated at 96.2% (25 of 26 strings)

Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_change_qty_reason
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_change_qty_reason/fr/
@liweijie0812 @JasminSForgeFlow
Translated using Weblate (Chinese (Simplified))
c804eed 
Currently translated at 100.0% (26 of 26 strings)

Translation: stock-logistics-warehouse-12.0/stock-logistics-warehouse-12.0-stock_change_qty_reason
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-12-0/stock-logistics-warehouse-12-0-stock_change_qty_reason/zh_CN/
@AdriaGForgeFlow @JasminSForgeFlow
[IMP] stock_change_qty_reason: black, isort
52d70da
@AdriaGForgeFlow @JasminSForgeFlow
[MIG] stock_change_qty_reason: Migration to 13.0
35f4da4
@oca-travis @JasminSForgeFlow
[UPD] Update stock_change_qty_reason.pot
465f127
@OCA-git-bot @JasminSForgeFlow
pre-commit update
7363ba3
@oca-transbot @JasminSForgeFlow
Update translation files
812b98c 
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: stock-logistics-warehouse-13.0/stock-logistics-warehouse-13.0-stock_change_qty_reason
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-13-0/stock-logistics-warehouse-13-0-stock_change_qty_reason/
@bizzappdev @JasminSForgeFlow
[FIX]make reason fields readonly if in done state
e7bf3b9
@OCA-git-bot @JasminSForgeFlow
stock_change_qty_reason 13.0.1.1.0
a0008b7
@GuillemCForgeFlow @JasminSForgeFlow
[IMP]stock_change_qty_reason: black, isort, prettier
16e1289
GuillemCForgeFlow and others added 15 commits September 17, 2024 16:45
@GuillemCForgeFlow @JasminSForgeFlow
[14.0][MIG]stock_change_qty_reason: Migration to 14.0
c6c662f
@oca-travis @JasminSForgeFlow
[UPD] Update stock_change_qty_reason.pot
25c84d6
@OCA-git-bot @JasminSForgeFlow
[UPD] README.rst
587c9b8
@OCA-git-bot @JasminSForgeFlow
stock_change_qty_reason 14.0.1.0.1
e2bf9d1
@rousseldenis @JasminSForgeFlow
[IMP] Update templates with copier
b91b19a
@OCA-git-bot @JasminSForgeFlow
stock_change_qty_reason 14.0.1.0.2
32f0367
@mymage @JasminSForgeFlow
Translated using Weblate (Italian)
fd77757 
Currently translated at 100.0% (25 of 25 strings)

Translation: stock-logistics-warehouse-14.0/stock-logistics-warehouse-14.0-stock_change_qty_reason
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-14-0/stock-logistics-warehouse-14-0-stock_change_qty_reason/it/
@MiquelRForgeFlow @JasminSForgeFlow
[MIG] stock_change_qty_reason: Migration to 15.0
e65dee6
@JasminSForgeFlow
[UPD] Update stock_change_qty_reason.pot
392c477
@OCA-git-bot @JasminSForgeFlow
[UPD] README.rst
3851640
@francesco-ooops @JasminSForgeFlow
Translated using Weblate (Italian)
347c7aa 
Currently translated at 100.0% (25 of 25 strings)

Translation: stock-logistics-warehouse-15.0/stock-logistics-warehouse-15.0-stock_change_qty_reason
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-15-0/stock-logistics-warehouse-15-0-stock_change_qty_reason/it/
@Urvisha-OSI @JasminSForgeFlow
[IMP] stock_change_qty_reason: pre-commit stuff
31da369
@Urvisha-OSI @JasminSForgeFlow
[MIG] stock_change_qty_reason: Migration to 16.0
d45420c
@paradoxxxzero @JasminSForgeFlow
[FIX] stock_change_qty_reason: Fix button_box in reasion view
2a4c1a2 
Also remove past migrations, and improve comments
@JasminSForgeFlow
[IMP] stock_change_qty_reason: black, isort, prettier
5bcc748
@JasminSForgeFlow JasminSForgeFlow force-pushed the 17.0-mig-stock_change_qty_reason branch from 30c419d to 15c3396 Compare September 17, 2024 11:40
@JasminSForgeFlow
Copy link
Contributor Author

Functional review. Preset reasons work well, the free text reasons are not usable though. With preset reasons disabled, you cannot edit the reason field while doing the inv. adjustment.
Also, not sure if both reasons fields should be displayed when using preset reasons.
image

The first reason is free text when we select any reason (2nd reason) in line while doing adjustment and click on apply, it will copy reason (2nd field) value to reason (1st field), and reason (2nd field) value set as False. reason(1st field) value is for historical purpose as reason (2nd filed) will be set False when adjustment done

@JasminSForgeFlow Sorry for the late reply.

If you want to keep it make it at least optional=hide, but in general it doesn't look very clean to me to keep both fields displayed. You should either go for preset reasons or not, if you starte with free text, you could do some custom migration or a custom module to show both reasons. In other words, we should think more on clean installations than on past sins or legacy issues.

Also, you haven't answered my second comment: the free text reason code is not editable even without pre-set reasons enabled.

this one is fixed.

LoisRForgeFlow
LoisRForgeFlow reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

I'm not able to write anything in the reason:

2024-09-17.16-52-56.mp4

@JasminSForgeFlow JasminSForgeFlow force-pushed the 17.0-mig-stock_change_qty_reason branch from 15c3396 to 180027a Compare September 18, 2024 04:08
@JasminSForgeFlow
Copy link
Contributor Author

I'm not able to write anything in the reason:

2024-09-17.16-52-56.mp4

its fixed, you can edit now

LoisRForgeFlow
LoisRForgeFlow approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Functional test, all god now 👍

Thanks!

@LoisRForgeFlow
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-2030-by-LoisRForgeFlow-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 52ce133 into OCA:17.0 Sep 18, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 8253cc5. Thanks a lot for contributing to OCA. ❤️

@LoisRForgeFlow LoisRForgeFlow deleted the 17.0-mig-stock_change_qty_reason branch September 18, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LoisRForgeFlow LoisRForgeFlow LoisRForgeFlow approved these changes

Assignees
No one assigned
Labels
merged 🎉 migration needs review
Projects
None yet
Milestone
17.0
Development

Successfully merging this pull request may close these issues.