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

[14.0][IMP] stock_picking_invoice_link #1010

Closed
wants to merge 5 commits into from
Closed

[14.0][IMP] stock_picking_invoice_link #1010

wants to merge 5 commits into from

Conversation

ajaniszewska-dev
Copy link

@ajaniszewska-dev ajaniszewska-dev commented Jun 1, 2022 •
edited
Loading

port changes from #816

@ajaniszewska-dev ajaniszewska-dev force-pushed the 14.0-imp-stock_picking_invoice_link_ordered branch 10 times, most recently from 76ebdf9 to ce51dc8 Compare June 2, 2022 14:11
grindtildeath
grindtildeath reviewed Jun 7, 2022
View reviewed changes
stock_picking_invoice_link/models/account_move.py Outdated
Comment on lines 19 to 20
relation="account_invoice_stock_picking_rel",
column1="account_invoice_id",
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we don't want to keep these values (using the defaults must be enough)

stock_picking_invoice_link/models/stock_picking.py Outdated Show resolved Hide resolved
@ajaniszewska-dev ajaniszewska-dev force-pushed the 14.0-imp-stock_picking_invoice_link_ordered branch from 51adb80 to eff5390 Compare June 20, 2022 07:04
@ajaniszewska-dev ajaniszewska-dev force-pushed the 14.0-imp-stock_picking_invoice_link_ordered branch from eff5390 to a4fcdcd Compare July 11, 2022 07:08
grindtildeath
grindtildeath approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@grindtildeath grindtildeath left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ajaniszewska-dev ajaniszewska-dev force-pushed the 14.0-imp-stock_picking_invoice_link_ordered branch from a4fcdcd to c58103e Compare July 11, 2022 08:30
i-vyshnevska
i-vyshnevska approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@i-vyshnevska i-vyshnevska left a comment

Choose a reason for hiding this comment

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

add the complete name of addon to the PR name and commits message stock_picking_invoice_link

stock_picking_invoice_link/views/account_invoice_view.xml Outdated
@@ -3,7 +3,7 @@
<!-- Copyright 2013-2014 Alexis de Lattre <[email protected]>
License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). -->
<record id="invoice_form" model="ir.ui.view">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<record id="invoice_form" model="ir.ui.view">
<record id="view_move_form" model="ir.ui.view">

Copy link
Author

Choose a reason for hiding this comment

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

this record id already exists in: https://github.com/OCA/stock-logistics-workflow/blob/14.0/stock_picking_invoice_link/views/stock_view.xml#L24
so i get:

[W7902(duplicate-xml-record-id), ] Duplicate xml record id "data/view_move_form_noupdate_0" in views/account_invoice_view.xml:5

i added "_inherit" postfix to avoid issues.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@ajaniszewska-dev ajaniszewska-dev changed the title 14.0 imp stock picking invoice link ordered [14.0][IMP] stock_picking_invoice_link Aug 2, 2022
@ajaniszewska-dev ajaniszewska-dev force-pushed the 14.0-imp-stock_picking_invoice_link_ordered branch 3 times, most recently from 5dd7704 to 09c8aff Compare August 2, 2022 07:02
@ajaniszewska-dev ajaniszewska-dev force-pushed the 14.0-imp-stock_picking_invoice_link_ordered branch from 09c8aff to f41fb82 Compare August 2, 2022 07:06
pedrobaeza
pedrobaeza requested changes Aug 2, 2022
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

My fears of #816 (comment) are still there

@i-vyshnevska
Copy link
Member

@pedrobaeza the problems emphasized in #816 (comment) is reasonable, but this PR doesn't touch that part of the code, this improvement to other parts of the addon
as for v14 addon was already merged what would be the point to keep less effective?

@pedrobaeza
Copy link
Member

Please check latest changes on 13.0 version, that should cover all the possible cases. The latest one arrived with #750. We should forward-port that changes instead. If there's something still uncovered, please comment.

@rousseldenis
Copy link
Contributor

/ocabot migration stock_picking_invoice_link

@OCA-git-bot OCA-git-bot added this to the 14.0 milestone Aug 19, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Aug 19, 2022
Open
57 tasks
@OCA-git-bot
Copy link
Contributor

The migration issue (#708) has been updated to reference the current pull request.
however, a previous pull request was referenced : #767.
Perhaps you should check that there is no duplicate work.
CC : @joao-p-marques

@grindtildeath
Copy link
Contributor

@pedrobaeza It's too bad I didn't got back to this PR during these OCA days to be able to talk about this issue together with you, and I'm not in Brussels but in the train on my way back to Switzerland.

I just tried the last changes that were merged in 14.0, but moves for product with ordered quantities are actually still not linked to the invoice.

About what you mentioned in #816 (comment) , to me the biggest issue was the change to a computed field instead of having everything stored, but it was already done some times ago.

Then you guys have been adding considerations of return moves to credit notes in #1100 which to me is the same kind of change of adding the support of ordered quantities.

Then AFAIU you also agreed in #989 (comment) that it doesn't make sense to keep this as a separated since the breaking change (i.e. the change to a computed field) was already merged.

Now, we will still need to rebase this PR and fix all the conflicts properly to consider the last changes that were done, but I don't want to spend any more time on this if the feature is going to stay in an unmerged PR.

Can we agree that we're going to merge this after rebase or should we better fix and merge #989 ?

@grindtildeath
Copy link
Contributor

ping @pedrobaeza
Can you please check my comment above? 🙏

@pedrobaeza
Copy link
Member

@CarlosRoca13 can you check?

@grindtildeath
Copy link
Contributor

ping @CarlosRoca13 🙏

@CarlosRoca13
Copy link
Contributor

Hi @grindtildeath

Sorry for not saying anything about it in this PR, We add the support to ordered quantities in this commit 0bd063a, you can check it and add any improvement that you need 😄

@grindtildeath
Copy link
Contributor

@CarlosRoca13 No worries, at least the feature is now available out of the box in the module and we don't need to spend any more time on it.

@ajaniszewska-dev We can then close this PR.

@grindtildeath grindtildeath mentioned this pull request Oct 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza requested changes

@i-vyshnevska i-vyshnevska i-vyshnevska approved these changes

@grindtildeath grindtildeath grindtildeath approved these changes

Assignees
No one assigned
Labels
enhancement needs review
Projects
None yet
Milestone
14.0
Development

Successfully merging this pull request may close these issues.
7 participants
@ajaniszewska-dev @OCA-git-bot @i-vyshnevska @pedrobaeza @rousseldenis @grindtildeath @CarlosRoca13