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

[MIG] account_move_name_sequence: Migration to 15.0 #1410

Merged

Conversation

frahikLV
Copy link
Contributor

Attendee #1253

@frahikLV
Copy link
Contributor Author

@luisg123v sorry for my mistake, fixed name branch on this PR

Can you review it?

cc: @moylop260

@frahikLV frahikLV force-pushed the 15.0-mig-account_move_name_sequence-dev-frahik branch from 0f8d792 to c199e23 Compare May 25, 2022 22:58
@frahikLV frahikLV mentioned this pull request May 25, 2022
31 tasks
"On journal '%s', the same sequence is used as "
"Entry Sequence and Credit Note Entry Sequence."
)
raise ValidationError(_(msg, journal.display_name))

Choose a reason for hiding this comment

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

Variables can't be translated

Copy link
Contributor

Choose a reason for hiding this comment

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

@frahikLV

It is weird, the original source is not using a variable:

@frahikLV why this change is required for this PR?

Could we do minimal changes here, please, in order to avoid diverging a lot from the original branch?

account_move_name_sequence/models/account_journal.py Outdated Show resolved Hide resolved
account_move_name_sequence/models/account_journal.py Outdated Show resolved Hide resolved
account_move_name_sequence/models/account_move.py Outdated Show resolved Hide resolved
invoice.unlink()
invoice.button_draft()
invoice.button_cancel()
with self.assertRaises(UserError):

Choose a reason for hiding this comment

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

You should use assertRaisesRegex instead whenever possible, to ensure the raised error is the expected one, e.g.:

error_msg = "could not perform the this operation because..."
with self.assertRaisesRegex(UserError, error_msg):
    ...

Same below.

else:
date_from = date_obj + relativedelta(day=1, month=1)
date_to = date_obj + relativedelta(day=31, month=12)
date_from = fields.Date.start_of(date_obj, "year")
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this refactoring should be done from v14.0 instead then FW to v15.0

In order to avoid diverging both versions

I mean, v14.0 supports the same method

Or better let it as it in v14.0 without changes in order to avoid this process

alexis-via and others added 23 commits May 26, 2022 15:31
This module restores the good old behavior where journal entry numbers
were generated from a sequence configured on the journal.
Add post-install script to create a sequence for all existing journals
Update README accordingly
…ange_day

More info about:
 - odoo/odoo#91019

TODO: Revert this commit after it is merged
After remove required=True for journal.sequence_id field it is possible to post an invoice with misconfigured journal with empty sequence

So, this constraint will raise an error for this kind of cases since that using '/' could raise the unique constraint for all other moves
The required flag was wrong for sequence_id and refund_sequence_id

So, it was not possible to store any change in journal for journal different to sale and purchase
@frahikLV frahikLV force-pushed the 15.0-mig-account_move_name_sequence-dev-frahik branch from c199e23 to fc3db9c Compare May 26, 2022 21:22
@frahikLV frahikLV force-pushed the 15.0-mig-account_move_name_sequence-dev-frahik branch from fc3db9c to e55ff74 Compare May 26, 2022 21:30
@frahikLV
Copy link
Contributor Author

@luisg123v All suggestions have been addressed, can you review again?

cc: @moylop260

@moylop260
Copy link
Contributor

moylop260 commented May 26, 2022

Future me:

The method _is_last_from_seq_chain calls _get_last_sequence method and it locks the last record

But it is only called from:

This method should be overwrite too in the future if it is called from another side in order to avoid useless locks

@moylop260
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 15.0-ocabot-merge-pr-1410-by-moylop260-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 23df84c into OCA:15.0 May 26, 2022
@OCA-git-bot
Copy link
Contributor

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

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.

5 participants