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][ADD] business code and validation #16

Merged
merged 2 commits into from
Sep 19, 2018

Conversation

jarmokortetjarvi
Copy link
Contributor

Adds a business id for partners.
Split to two modules: one just adds the field and other adds validation and formatting.
This way the validation will be optional, and the module can be installed in environments with incorrectly formatted business ids

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 98.333% when pulling 51fa60e on Tawasta:10.0-l10n_fi_business_code into 5aee123 on OCA:10.0.

@coveralls
Copy link

coveralls commented Aug 9, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 1240cca on Tawasta:10.0-l10n_fi_business_code into 19b1749 on OCA:10.0.

Copy link
Contributor

@aisopuro aisopuro left a comment

Choose a reason for hiding this comment

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

My concerns on the previous version of this pull request (#15) were already addressed. This is OK.

README.md Outdated
@@ -13,6 +13,7 @@ Available addons
addon | version | summary
--- | --- | ---
[l10n_fi_banks](l10n_fi_banks/) | 10.0.1.0.0 | Finnish banks and their addresses
[l10n_fi_edicode](l10n_fi_edicode/) | 10.0.1.1.2 | Adds EDI code field and operators

Choose a reason for hiding this comment

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

Maybe add l10n_fi_business_code and l10n_fi_business_code_validate here too.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Maybe remove this modification (edicode) and let OCA bot autogenerate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. The auto-generation was the reason why I didn't add the business code -modules in readme.

10.0.20180727.0

Choose a reason for hiding this comment

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

Is there a reason why all this setup/ stuff is in here? Isn't this something that OCA Bot generates automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, they are autogenerated for https://pypi.org/project/setuptools-odoo/
They seem to be here due to a merge 4627715 from main branch

Choose a reason for hiding this comment

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

Ok. Could you remove them from this PR?

Choose a reason for hiding this comment

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

@aisopuro Do you know how we could get rid of those commits? I tried to remove them by using interactive rebase but failed miserably.

Copy link
Contributor

@aisopuro aisopuro Aug 29, 2018

Choose a reason for hiding this comment

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

It looks a little hairy. There are some merge commits in there, which could make the whole thing really complicated. Maybe try a reset-"rebase":

  1. git checkout -b just-testing 10.0-l10n_fi_business_code
  2. git fetch origin
  3. git reset --soft origin/10.0 (Assuming origin is the remote pointing to the OCA repository)
  4. git add the-things-you-want-here (make sure you don't add any setup/ stuff if it isn't wanted)
  5. git commit
  6. (check the branch makes sense, then reset --hard your feature branch to point to this fixed branch)
  7. git push --force-with-lease

This should "squash" everything so that you only have one commit with the files you want to have. You can also do steps 4 and 5 several times, if you want to have different commits cover different things.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mlaitinen it looks like your review is maybe the only thing holding this back.

@jarmokortetjarvi
Copy link
Contributor Author

Tried to clean up the commit history. If everything went as planned, it should be identical to the original PR (with fixes of course), but without any ./setup-files.

Pre-cleanup branch for reference https://github.com/Tawasta/l10n-finland/tree/10.0-l10n_fi_business_code_original

@mlaitinen mlaitinen merged commit cd91bbc into OCA:10.0 Sep 19, 2018
aisopuro referenced this pull request in avoinsystems/l10n-finland Sep 21, 2018
* Add a module for partner business id (business code)
* Add a module for partner business id (business code) validation
@jarmokortetjarvi jarmokortetjarvi deleted the 10.0-l10n_fi_business_code branch December 5, 2019 09:07
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.

4 participants