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

Internal_number in list view of invoice #314

Merged
merged 2 commits into from Jul 26, 2016

Conversation

sadamo
Copy link
Contributor

@sadamo sadamo commented May 19, 2016

No description provided.

@renatonlima
Copy link
Member

👍

@rvalyi
Copy link
Member

rvalyi commented May 29, 2016

👎 replacing a field with another is not a good practise I think. The reason is that some module could expect this field to be present to compute an other field or a color, I remember we had views errors in the old days of the localization when we were doing that kind of things... I prefer an extension that would set this field as invisible and add the internal_number after or before it.

@sadamo sadamo force-pushed the oca/internal-number-in-tree-view branch 2 times, most recently from 507b0f0 to a1841be Compare June 8, 2016 15:43
@sadamo
Copy link
Contributor Author

sadamo commented Jun 10, 2016

@rvalyi, good point. I've altered that and squashed the commit.

@rvalyi
Copy link
Member

rvalyi commented Jun 20, 2016

👍 thanks @sadamo

@mileo
Copy link
Member

mileo commented Jun 21, 2016

👍

@renatonlima
Copy link
Member

👍 @rvalyi good catch!

@@ -43,6 +43,7 @@

class AccountInvoice(models.Model):
_inherit = 'account.invoice'
_order = "internal_number desc, id desc"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should find a better way to sort this, as internal_number is a char field, this does not work as expected.
broken order

Copy link
Member

Choose a reason for hiding this comment

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

I think we can order by date_intoice and internal_number

Copy link
Member

Choose a reason for hiding this comment

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

@sadamo why you add _order property in l10n_br_account and l10n_br_account_product, I can add _order property only in l10n_br_account is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@renatonlima because the field date_hour_invoice is declared in l10n_br_account_product. But if using date_invoice to order, which now I think should be the best, the l10n_br_account_product _order can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Best way is to use date_invoice , because in account.invoice CAN be used to create other fiscal document types like NFS-e, using date_invoice, you can remove _order in l10n_br_account_product

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something strange happened when I use the _order property only in l10n_br_account the way we are discussing, it doesn't update the view, even in new db.
Only in l10n_br_account_product this work. Anyone knows something about?

Copy link
Member

Choose a reason for hiding this comment

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

@sadamo the problem is in the line 127 there is another definition of the attribute _order https://github.com/kmee/l10n-brazil/blob/916ad3cf2e80d1e755972034ddc8943f4d34c061/l10n_br_account/models/account_invoice.py#L127

You can remove the line 127 and change _order atribute to: _order = "date_invoice desc, internal_number desc" and in l10n_br_account_product chante to _order = "date_hour_invoice desc, internal_number desc"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only used the _order property in the l10n_br_account with: "date_invoice desc, internal_number desc". Because if used the date_hour_invoice there will not be an sort, I think it will became confuse.

@rvalyi
Copy link
Member

rvalyi commented Jun 21, 2016

@sadamo could you please rebase on 8.0 ? I also think @danimaribeiro made a good point...

@rvalyi rvalyi added the 8.0 label Jun 21, 2016
…l_number

[IMP] Invoice tree view using internal_number in place of number field

[IMP] invoices in tree view ordered by internal_number and date_hour_invoice

correcao
@sadamo sadamo force-pushed the oca/internal-number-in-tree-view branch from a1841be to 916ad3c Compare July 14, 2016 14:55
@mileo
Copy link
Member

mileo commented Jul 14, 2016

Rebased

@mileo
Copy link
Member

mileo commented Jul 19, 2016

👍 again

@@ -37,6 +37,7 @@

class AccountInvoice(models.Model):
_inherit = 'account.invoice'
_order = "internal_number desc, date_hour_invoice desc"
Copy link
Member

Choose a reason for hiding this comment

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

To correct order you must invert the order of columns: _order = "date_hour_invoice desc, internal_number desc"

@sadamo
Copy link
Contributor Author

sadamo commented Jul 25, 2016

Altered the _order to "date_invoice desc, internal_number desc". Ready to merge?

@rvalyi
Copy link
Member

rvalyi commented Jul 26, 2016

@renatonlima @sadamo didn't we agree to override the _order and use date_hour_invoice in l10n_br_product for more precision and minimize possible order errors in case of several invoices the same day with the string sorting issue raised by @danimaribeiro ?

@sadamo
Copy link
Contributor Author

sadamo commented Jul 26, 2016

I have made some tests and with date_hour_invoice in _order the list will have no sorted sequence, I think the date_invoice is better. In the firsts numbers we can use the ir_sequence to complete with zeros. What do you think? @renatonlima @danimaribeiro @rvalyi

@danimaribeiro
Copy link
Contributor

I don't get it:
"date_hour_invoice in _order the list will have no sorted sequence"

@renatonlima
Copy link
Member

Hi guys,

@sadamo as I said in l10n_br_account_product we can keep _order = 'date_hour_invoice DESC, internal_number DESC' to minimize possible order errors in case of several invoices the same. I think it is enough.

@sadamo
Copy link
Contributor Author

sadamo commented Jul 26, 2016

Ops, my bad, I mixed with another feature. Will change to date_hour_invoice.

…l_number desc

[FIX] Sort by  date_hour_invoice desc, internal_number desc in invoice tree view
@sadamo sadamo force-pushed the oca/internal-number-in-tree-view branch from 032260c to 433dc5e Compare July 26, 2016 17:53
@sadamo
Copy link
Contributor Author

sadamo commented Jul 26, 2016

Done!

@renatonlima
Copy link
Member

LGTM 👍

@rvalyi
Copy link
Member

rvalyi commented Jul 26, 2016

👍 thanks @sadamo!

@mbcosta
Copy link
Contributor

mbcosta commented Jul 26, 2016

👍

@renatonlima renatonlima merged commit 433dc5e into OCA:8.0 Jul 26, 2016
@mileo mileo deleted the oca/internal-number-in-tree-view branch August 8, 2018 21:11
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.

None yet

6 participants