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

[11.0][MIG] account_reconcile_rule (account_operation_rule v9) #209

Closed
wants to merge 6 commits into from
Closed

[11.0][MIG] account_reconcile_rule (account_operation_rule v9) #209

wants to merge 6 commits into from

Conversation

grindtildeath
Copy link
Contributor

@grindtildeath grindtildeath commented Apr 23, 2018 •
edited
Loading

Todo :

  • Fix Javascript
  • Check readme

@grindtildeath grindtildeath force-pushed the 11.0-mig-account_operation_rule branch 4 times, most recently from 4419041 to 5cfe225 Compare April 25, 2018 15:27
@grindtildeath grindtildeath changed the title [WIP][11.0][MIG] account_operation_rule > account_reconcile_rule [11.0][MIG] account_operation_rule > account_reconcile_rule Apr 25, 2018
@grindtildeath grindtildeath force-pushed the 11.0-mig-account_operation_rule branch 2 times, most recently from 8b1d1af to 47b862c Compare April 25, 2018 15:50
@grindtildeath grindtildeath changed the title [11.0][MIG] account_operation_rule > account_reconcile_rule [11.0][MIG] account_reconcile_rule (account_operation_rule v9) Apr 25, 2018
simahawk
simahawk requested changes Apr 26, 2018
View reviewed changes
account_reconcile_rule/__manifest__.py Outdated
@@ -0,0 +1,23 @@
# Author: Guewen Baconnier
# © 2014-2016 Camptocamp SA
Copy link

Choose a reason for hiding this comment

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

Copyright

account_reconcile_rule/__manifest__.py Outdated
'depends': [
'account',
],
'website': 'http://www.camptocamp.com',
Copy link

Choose a reason for hiding this comment

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

https://github.com/OCA/account-reconcile

account_reconcile_rule/__manifest__.py Outdated
'view/account_reconcile_rule_view.xml',
'security/ir.model.access.csv',
],
'installable': True,
Copy link

Choose a reason for hiding this comment

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

you can drop these flags

account_reconcile_rule/i18n/account_statement_operation_rule.pot Outdated
@@ -0,0 +1,152 @@
# Translation of Odoo Server.
Copy link

Choose a reason for hiding this comment

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

remove POT

account_reconcile_rule/model/__init__.py Outdated
@@ -0,0 +1,4 @@

Copy link

Choose a reason for hiding this comment

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

empty line not needed

account_reconcile_rule/model/account_reconcile_rule.py Outdated
@api.model
@api.returns('account.reconcile.model')
def models_for_reconciliation(self, statement_line_id, move_line_ids):
""" Find the rule for the current reconciliation and returns the
Copy link

Choose a reason for hiding this comment

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

"""Find reconcile models for given statement and move lines.
explain "how" in the extra lines.

account_reconcile_rule/static/src/js/account_widgets.js

var core = require('web.core');
var reconciliation_renderer = require('account.ReconciliationRenderer');

Copy link

Choose a reason for hiding this comment

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

/*
Explain what you do here 
*/`

account_reconcile_rule/static/src/js/account_widgets.js Outdated
return deferred;
},
/**
* Click on add_line link if preset button have been clicked
Copy link

Choose a reason for hiding this comment

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

???

account_reconcile_rule/tests/common.py Outdated
'code': 'CASH',
'type': 'cash',
})

Copy link

Choose a reason for hiding this comment

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

remove some useless empty line

account_reconcile_rule/tests/common.py Outdated

@classmethod
def setUpClass(cls):
super(AccountReconciliationModelTestCase, cls).setUpClass()
Copy link

Choose a reason for hiding this comment

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

you can call super() w/out args

guewen
guewen approved these changes Apr 26, 2018
View reviewed changes
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.

Code LGTM (only some commented lines to remove), tested on runbot, works great. Thanks!

account_reconcile_rule/static/src/js/account_widgets.js Outdated
// var deferred = this._super();
// deferred.done(this.operation_rules());
// return deferred;
// }
Copy link
Member

Choose a reason for hiding this comment

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

I guess the commented lines can be removed

@grindtildeath grindtildeath force-pushed the 11.0-mig-account_operation_rule branch from 47b862c to 009122c Compare April 26, 2018 08:30
@grindtildeath grindtildeath mentioned this pull request Apr 26, 2018
Closed
3 tasks
@grindtildeath grindtildeath force-pushed the 11.0-mig-account_operation_rule branch from 009122c to f9a850f Compare April 26, 2018 11:36
tschanzt
tschanzt reviewed Apr 30, 2018
View reviewed changes
Copy link

@tschanzt tschanzt left a comment

Choose a reason for hiding this comment

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

Some of the Headers have the wrong Daterange in it for the licence. But otherwise 👍

@emagdalenaC2i emagdalenaC2i mentioned this pull request Dec 28, 2018
Closed
26 tasks
@grindtildeath grindtildeath mentioned this pull request Feb 21, 2019
Merged
@grindtildeath
Copy link
Contributor Author

@tschanzt Can you approve or add suggestions to correct the date range so we can get this merged ? 🙏 😉

@simahawk
Copy link

@tschanzt ping

@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). 🤖

@benbrich
Copy link

@fclementic2c how about an merge here? Everythings seems allright from over here. Thx!

@benbrich
Copy link

benbrich commented Jul 4, 2019

@fclementic2c sorry for poking you on this one, I have learned from @OSevangelist that you are more into funtional stuff. @tafaRU or @jbeficent could you please be so kind and merge this one? Thanks!

@pedrobaeza
Copy link
Member

/ocabot merge

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Rebased to 11.0-ocabot-merge-pr-209-by-pedrobaeza-bump-no, awaiting test results.

@OCA-git-bot
Copy link
Contributor

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

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 11.0.

OCA-git-bot added a commit that referenced this pull request Jul 23, 2019
@OCA-git-bot
Merge PR #209 into 11.0
e70bb0f 
Signed-off-by pedrobaeza
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simahawk simahawk simahawk approved these changes

@tschanzt tschanzt tschanzt approved these changes

@guewen guewen guewen approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@grindtildeath @simahawk @OCA-git-bot @benbrich @pedrobaeza @tschanzt @guewen @oca-transbot