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_batch_validate: Migration to 9.0 #602

Closed
wants to merge 4 commits into from
Closed

[MIG] account_move_batch_validate: Migration to 9.0 #602

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 16, 2018

No description provided.

@pedrobaeza pedrobaeza mentioned this pull request Jan 16, 2018
38 tasks
@pedrobaeza
Copy link
Member

Please check Travis. There's no extra commits to rescue from 8.0? Check migration guide: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-9.0

Copy link

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

code review only: LGTM. Just a bunch of more or less nitpicking remarks.

'author': "Camptocamp,Odoo Community Association (OCA)",
'category': 'Finance',
'complexity': 'normal',
'depends': [
'account',
'account_default_draft_move',
'account_accountant',
'connector',
],
'website': 'http://www.camptocamp.com',

Choose a reason for hiding this comment

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



class AccountMove(models.Model):

Choose a reason for hiding this comment

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

drop this empty line

return job

def test_batch_validate(self):

Choose a reason for hiding this comment

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

can you skip some empty lines?

@simahawk
Copy link

@OleksandrPaziuk pls, check Pedro's comment about history.

@ghost
Copy link
Author

ghost commented Jan 23, 2018

There are only 5 OCA Transbot updated translations from Transifex commits. I don't sink we need them for history.

@pedrobaeza
Copy link
Member

Yes, we need them because they are translations, but you have to squash them together. What we don't want is that detailed history. Please follow https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-9.0 and https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests guides

guewen
guewen previously approved these changes Jan 31, 2018
def validate_one_move(session, model_name, move_id):
"""Validate a move, and leave the job reference in place."""
move_ids = session.env[model_name].browse([move_id])
if move_ids.exists():
Copy link
Member

Choose a reason for hiding this comment

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

Code could be a bit simpler IMO: here we could check if to_post is still True, if not just leave with returning a message saying the move is not to post anymore. Thus we can remove the unmark_for_posting and _cancel_jobs methods. Anyway, this is a migration so maybe this PR is not the right place to change.

Copy link
Member

@guewen guewen left a comment

Choose a reason for hiding this comment

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

No commits for the original authors?

@ghost
Copy link
Author

ghost commented Jan 31, 2018

@guewen Unfortunately, someone incorrectly moves the module from 8 to 9 (before me) and therefore commits did not remain in history. Just in case, check if you want.

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 31, 2022
@github-actions github-actions bot closed this Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants