[ADD] [10.0] [BR001136] [23300] account_invoice_kanban #309
Conversation
|
@elicoidal Could you please review code. |
nikul-serpentcs
force-pushed
the
10.0-account_invoice_kanban
branch
from
c40bbf8
to
972f9ed
Compare
| ======================= | ||
| Account Invoice Kanban | ||
| ======================= | ||
|
|
There was a problem hiding this comment.
Thanks @nikul-serpentcs
IMO this should use base_kanban_stage, which would reduce the need for boilerplate code.
I won't block for that, but I do feel that the lack of automated testing is a problem 👎
Edit: link
| <field name="sequence">4</field> | ||
| <field name="name">Documentation Sent</field> | ||
| </record> | ||
| <record id="invoice_recrived" model="account.invoice.stage"> |
|
Ok wait maybe I'm confused here - I assumed that we would have the standard Kanban logic, such as the default stage and Also - why are we introducing entirely new invoice tree views? |
I dont know the module and not against using it |
Indeed: there is no functional reason for it at least |
|
Hello, @lasley |
@lasley Okay then we'll use the base_kanban_stage, so I guess then it will be fine, right? And also we need some help to use it! |
It's so simple! There's some usage instructions here. Here's an example of a model that has stages, and here's the view. |
|
@elicoidal @lasley |
|
@nikul-serpentcs After tested here is my feedback:
Can you review it? |
@elicoidal stages are managed, go to Settings > Technical > Kanban > Stages. |
account_invoice_kanban/README.rst
Outdated
| ======================= | ||
|
|
||
|
|
||
| Introduction |
There was a problem hiding this comment.
Remove this header please - introduction is assumed to be the text immediately below the main header
account_invoice_kanban/README.rst
Outdated
| ============= | ||
| Stage are set up in the Invoicing application: | ||
|
|
||
| #. Invoicing menu --> Configuration --> Stages --> Invoice Stages |
account_invoice_kanban/README.rst
Outdated
|
|
||
| Known Issues / Roadmap | ||
| ====================== | ||
| * Stages are somehow redondant with invoice status: it would be good to have the option to |
| _order = 'date_due asc' | ||
|
|
||
| @api.multi | ||
| def _read_group_stage_ids(self, stages, domain, order): |
There was a problem hiding this comment.
This is implemented in the base & doesn't look to be changed here. I think you can delete
There was a problem hiding this comment.
@nikul-serpentcs I agree with @lasley.
You inherit base.kanban.abstract and here the original code:
@api.multi
def _read_group_stage_ids(self, stages, domain, order):
search_domain = [('res_model_id.model', '=', self._name)]
if domain:
search_domain.extend(domain)
return stages.search(search_domain, order=order)
Then here:
@api.multi
def _read_group_stage_ids(self, stages, domain, order):
search_domain = [('res_model_id.model', '=', self._name)]
return stages.search(search_domain, order=order)
So the only real change is you remove the if condition, may I ask why?
There was a problem hiding this comment.
@victormartinelicocorp Yes the code was changed a long time ago, you are referring to the old code.
Please check the latest changes where the @lasley comment is attended, thanks!
There was a problem hiding this comment.
@darshan-serpent Could you help me with the review and give the link to the comment on @lasley you mention?
About the code
I am looking at this: https://github.com/OCA/account-invoicing/pull/309/files#diff-f0c2828b9fe5cf1e5f1cd28f2a52f4c2
And this: https://github.com/OCA/server-tools/blob/10.0/base_kanban_stage/models/base_kanban_abstract.py#L109
I am not necessary subscribing all opinions of others reviewers so please attend my concern that is:
- Here you are overwriting a function that seems not been changed just for an if condition that perhaps is necessary.
I am not asking for change the code if you give a reason.
There was a problem hiding this comment.
This can be removed now due to OCA/server-tools#989
nikul-serpentcs
force-pushed
the
10.0-account_invoice_kanban
branch
2 times, most recently
from
b1bd79a
to
daeabed
Compare
|
@lasley I removed the domain parameter from the method as it was catching the domain from the invoice action, which eventually showed me the below error: |
|
@nikul-serpentcs are you referring to Also please add automated tests for all logic. In the instance of your |
|
Another little fix to add: when copying an invoice, the stage should be reset to draft (first in the sequence) |
|
@lasley @elicoidal |
|
@nikul-serpentcs did you fix #309 (comment)? |
|
and #309 (comment)? |
|
@elicoidal Added Demo data file. |
|
Hmmm so this is in the base module, and it's probably because you don't have debug mode active (it's in the Tech Settings). For the most part we've found that the KanBan menus are configured by implementers using the main menu, but the users all seem to be managing directly in the KanBan view. Do you disagree with this evaluation? |
@elicoidal Only payment received stage are folded other stages are not folded. |
nikul-serpentcs
force-pushed
the
10.0-account_invoice_kanban
branch
from
69b9efe
to
88708e8
Compare
|
@elicoidal @lasley |
|
FYI, Otherwise the following js error is raised: So please review it and merge it asap. Thanks! |
|
OCA/server-tools#949 merged. @nikul-serpentcs can you amend your last commit and force push to trigger a build on the new base, please and thanks 😄 |
nikul-serpentcs
force-pushed
the
10.0-account_invoice_kanban
branch
from
a76be1f
to
0c5c050
Compare
|
@elicoidal @lasley Amended changes, can you please review it Thanks 😄 |
|
@tafaRU @lasley thanks a lot! After that, I think we are good to go |
| _order = 'date_due asc' | ||
|
|
||
| @api.multi | ||
| def _read_group_stage_ids(self, stages, domain, order): |
There was a problem hiding this comment.
@nikul-serpentcs I agree with @lasley.
You inherit base.kanban.abstract and here the original code:
@api.multi
def _read_group_stage_ids(self, stages, domain, order):
search_domain = [('res_model_id.model', '=', self._name)]
if domain:
search_domain.extend(domain)
return stages.search(search_domain, order=order)
Then here:
@api.multi
def _read_group_stage_ids(self, stages, domain, order):
search_domain = [('res_model_id.model', '=', self._name)]
return stages.search(search_domain, order=order)
So the only real change is you remove the if condition, may I ask why?
|
@victormartinelicocorp Code has been changed |
|
@victormartinelicocorp Yes, we overwrote the function and removed the As if we don't pass the domain blank, it was showing error. The domain included was As the base Please refer domain error and lasley comment |
|
@darshan-serpent - That makes sense, but why can we not just call the super with a blank domain then? |
|
@elicoidal - do you mind if we squash your ReadMe updates into @nikul-serpentcs commits to save some changelog? If not, can you please squash at your leisure for merge? |
| _order = 'date_due asc' | ||
|
|
||
| @api.multi | ||
| def _read_group_stage_ids(self, stages, domain, order): |
There was a problem hiding this comment.
Oops I added this to a collapsed thread:
This can be removed now due to OCA/server-tools#989
|
@lasley @elicoidal Removed read_group method, Could you please look it. |
no problem |
nikul-serpentcs
force-pushed
the
10.0-account_invoice_kanban
branch
from
90b2eb7
to
47fe0a5
Compare
|
@elicoidal Done squashed all commits. |
|
Functional tests OK: for me good to go! |
Migrated 'account_invoice_kanban' module for kanban view of invoices like customer invoice and supplier invoice.