Added support for Norwegian bank account numbers #275
Conversation
There was a problem hiding this comment.
Since this is adding a new feature, this PR will be included in version 1.5. I'm going try to be more regular about releasing new versions so you shouldn't have to wait 6 months to get this change in a released version.
We use Transifex for translations so please remove the translations changes from this PR. The translations will be synced to Transifex after the PR is merged.
There is an IBAN specified for Noway. It it common that people use the non-IBAN version of the bank account number?
| raise ValidationError(self.error_messages['invalid_length']) | ||
|
|
||
| # Adhere for the special rule of these being "clearing" accounts | ||
| if bank_number[4:5] == "00": |
There was a problem hiding this comment.
bank_number[4:5] can never be 2 characters. Did you mean to write bank_number[4:6]? I found this because I noticed that line 145 isn't covered by the tests.
There was a problem hiding this comment.
Thanks for catching that!
I've looked into the clearing numbers, and it's been a struggle to find any more details about exactly how to validate these, so I'm removing it and adding a note about bank clearing numbers in the function docstring.
|
Please also add a changelog entry for this PR. Thanks. |
Includes tests, update to docs and uses to_python, validate and prepare_value only.
|
I've updated with a less messy version that does the same thing without overriding I've updated the @benkonrath Extremely so. IBANs are only used in the context of international transactions, and never otherwise referred to domestically. |
|
|
||
| def to_python(self, value): | ||
| value = super(NOBankAccountNumber, self).to_python(value) | ||
| return value.replace(".", "").replace(" ", "") |
There was a problem hiding this comment.
Can you change the double quotes to single quotes just to be consistent with the rest of the file?
| return value.replace(".", "").replace(" ", "") | ||
|
|
||
| def prepare_value(self, value): | ||
| return "{}.{}.{}".format(value[0:4], value[4:6], value[6:11]) |
There was a problem hiding this comment.
I think it's worth adding a test for the formatting.
There was a problem hiding this comment.
I've added a simple test to validate it. 👍
We rely on the validation to prevent faulty values from entering the form in the first place.
|
Thanks again for your contribution! |
|
Thanks for the helpful feedback! 👍 |
|
@benkonrath Hi Ben! Sorry for bumping this, but I'm wondering when you're looking to release a new version? I feel like releasing small and often might be preferable to releasing only with new Django releases, or similarly. :) |
|
@thor No problem. Thanks for checking in. I was actually planning to do a release this week but other commitments got in the way. I make sure I get it out next week. |
This includes tests that cover it up fairly well, as well as some documentation to go along.
I
makemessages -l nbandcompilemessages -l nb, but if you import from Transifex either way, then I can do the changes there. 🌞I haven't been able to figure out exactly what the roadmap is; while it seemed to be a link with Django's roadmap, it also seemed like you're free to release when you've got a worthwhile release, and so I tossed it on a hope of 1.4.2. Admittedly I can change that to 1.5 if need be. 👍
All Changes
Add an entry to the docs/changelog.rst describing the change.
Add an entry for your name in the docs/authors.rst file if it's not
already there.
Adjust your i ports to a standard form by running this command:
New Fields Only
Prefix the country code to all fields.
Field names should be easily understood by developers from the target
localflavor country. This means that English translations are usually
not the best name unless it's for something standard like postal code,
tax / VAT ID etc.
Prefer 'PostalCodeField' for postal codes as it's
international English; ZipCode is a term specific to the United
States postal system.
Add meaningful tests. 100% test coverage is not required but all
validation edge cases should be covered.
Add
.. versionadded:: <next-version>comment markers to newlocalflavors.
Add documentation for all fields.