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

[10.0] Port base modules to v10 #164

Merged
merged 13 commits into from
Mar 6, 2017
Merged

Conversation

alexis-via
Copy link
Contributor

This is a work in progress... it doesn't work yet, because I don't know how to port "self._fields[k]._symbol_set" to v10, cf file account_move_base_import/models/account_move.py method _prepare_insert()

@alexis-via alexis-via mentioned this pull request Nov 9, 2016
23 tasks
@alexis-via
Copy link
Contributor Author

Account_move_base_import and base_transaction_id should work fine on v10 with my latest commit dated Nov 13.

@lmignon
Copy link
Sponsor

lmignon commented Jan 3, 2017

@alexis-via Can you check to the errors on travis?

@leemannd
Copy link
Contributor

leemannd commented Jan 4, 2017

@alexis-via If you cherry-pick from #165 you will remove an error related to base_transaction_id

@leemannd
Copy link
Contributor

leemannd commented Jan 4, 2017

@lmignon @pedrobaeza The error is related to missing workflow imported here https://github.com/OCA/bank-statement-reconcile/blob/10.0/account_move_base_import/test/refund.yml#L30 Just a question, these tests should be updated no?

@alexis-via
Copy link
Contributor Author

I don't plan to work on this PR in the short term (too much work, other priorities). I'll just merge the PR that I may receive. I you prefer to override it with another PR, no pb. It's just a pity to duplicate the porting work... but now that it's done, let's move forward.

oca-transbot and others added 4 commits January 11, 2017 13:54
OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex

OCA Transbot updated translations from Transifex
@leemannd
Copy link
Contributor

@lmignon A PR on the @alexis-via 's branch has been done to update this.

Copy link
Contributor

@leemannd leemannd 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

@ghost
Copy link

ghost commented Feb 20, 2017

Hi guys, what's the state of this PR? Is there still work to do? Otherwise, I suggest you merge it.

@ghost
Copy link

ghost commented Mar 2, 2017

@leemannd Hi leemannd, maybe you didn't see my last message. Is there still work to do for this PR? If not, I suggest you merge it. There are other PRs like OCA/l10n-switzerland#295 which are waiting for this one.

@leemannd
Copy link
Contributor

leemannd commented Mar 2, 2017

@BT-faebi Hello, I'm not the initial worker on this PR and nor a PSC member. I made what I could to correct it. I suggest you to make a code review and give a feedback if it's missing something. It would help in the process of merging it.

@guewen guewen merged commit 7f32a38 into OCA:10.0 Mar 6, 2017
@ghost
Copy link

ghost commented Apr 10, 2017

@leemannd I can only give my opinion on the code related to base_transaction_id (as I've never used account_move_base_import). But the code of base_transaction_id looks OK to me.

BTW, in the future it would be good to separate PRs by module, especially in this particular case where base_transaction_id and account_move_base_import are not depending on each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants