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

Map text according to EPC SEPA conversion table #105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kiriakosv
Copy link

Instead of whitelisting certain characters, we map according to EPC SEPA conversion guidelines.

Resolves #92

spec/converter_spec.rb Outdated Show resolved Hide resolved
@olleolleolle
Copy link
Contributor

I think this is stunning in its clarity, following the Best Practices outlined by the EPC.

@ledermann
Copy link
Member

ledermann commented Jun 8, 2022

Hey guys, thanks for your great work!
To minimize modifications in the existing specs, I suggest adding the following changes:

diff --git a/lib/sepa_king/converter.rb b/lib/sepa_king/converter.rb
index ead034f..9dbea7e 100644
--- a/lib/sepa_king/converter.rb
+++ b/lib/sepa_king/converter.rb
@@ -23,7 +23,7 @@ module InstanceMethods
       private_constant :EPC_MAPPING

       def convert_text(value)
-        return unless value
+        return value unless value.present?

         # Map chars according to:
         # http://www.europeanpaymentscouncil.eu/index.cfm/knowledge-bank/epc-documents/sepa-requirements-for-an-extended-character-set-unicode-subset-best-practices/
diff --git a/spec/converter_spec.rb b/spec/converter_spec.rb
index 6f0521a..0219fa3 100644
--- a/spec/converter_spec.rb
+++ b/spec/converter_spec.rb
@@ -26,6 +26,10 @@ it 'should convert number' do
       expect(convert_text(1234)).to eq('1234')
     end

+    it 'should not touch valid chars' do
+      expect(convert_text("abc-ABC-0123- :?,-(+.)/")).to eq("abc-ABC-0123- :?,-(+.)/")
+    end
+
     it 'should not touch nil' do
       expect(convert_text(nil)).to eq(nil)
     end
diff --git a/spec/transaction_spec.rb b/spec/transaction_spec.rb
index d7f8d01..30f5730 100644
--- a/spec/transaction_spec.rb
+++ b/spec/transaction_spec.rb
@@ -68,7 +68,7 @@ it 'should accept valid value' do
     end

     it 'should not accept invalid value' do
-      expect(SEPA::Transaction).not_to accept('', for: :name)
+      expect(SEPA::Transaction).not_to accept('', {}, for: :name)
     end
   end

What do you think?

@kiriakosv
Copy link
Author

kiriakosv commented Jun 8, 2022

Hey guys, thanks for your great work! To minimize modifications in the existing specs, I suggest adding the following changes:

diff --git a/lib/sepa_king/converter.rb b/lib/sepa_king/converter.rb
index ead034f..9dbea7e 100644
--- a/lib/sepa_king/converter.rb
+++ b/lib/sepa_king/converter.rb
@@ -23,7 +23,7 @@ module InstanceMethods
       private_constant :EPC_MAPPING

       def convert_text(value)
-        return unless value
+        return value unless value.present?

         # Map chars according to:
         # http://www.europeanpaymentscouncil.eu/index.cfm/knowledge-bank/epc-documents/sepa-requirements-for-an-extended-character-set-unicode-subset-best-practices/
diff --git a/spec/converter_spec.rb b/spec/converter_spec.rb
index 6f0521a..0219fa3 100644
--- a/spec/converter_spec.rb
+++ b/spec/converter_spec.rb
@@ -26,6 +26,10 @@ it 'should convert number' do
       expect(convert_text(1234)).to eq('1234')
     end

+    it 'should not touch valid chars' do
+      expect(convert_text("abc-ABC-0123- :?,-(+.)/")).to eq("abc-ABC-0123- :?,-(+.)/")
+    end
+
     it 'should not touch nil' do
       expect(convert_text(nil)).to eq(nil)
     end
diff --git a/spec/transaction_spec.rb b/spec/transaction_spec.rb
index d7f8d01..30f5730 100644
--- a/spec/transaction_spec.rb
+++ b/spec/transaction_spec.rb
@@ -68,7 +68,7 @@ it 'should accept valid value' do
     end

     it 'should not accept invalid value' do
-      expect(SEPA::Transaction).not_to accept('', for: :name)
+      expect(SEPA::Transaction).not_to accept('', {}, for: :name)
     end
   end

What do you think?

This is about the desired behaviour. Do we want to convert an empty hash to '()', as it currently does in this PR, or keep the previous behaviour (validation error)? I think I agree with you, we should revert back to the previous behaviour.

Also, something else; am I correct to assume that this block belongs to the Name context? If so, I could add the empty hash case here and remove the misplaced block.

@ledermann
Copy link
Member

Also, something else; am I correct to assume that this block belongs to the Name context? If so, I could add the empty hash case here and remove the misplaced block.

Yes, seems reasonable.

@fpietka
Copy link

fpietka commented Aug 3, 2022

I was wondering: in the official conversion table they suggest both a "basic" and a "long term" replacement. Won't there be any issue introducing html entities (containing & character) if, maybe, the bank isn't ready to process this character at all?

It may be a non issue as I haven't tested it myself, but I thought it would we worth talking about.
What do you think?

@fpietka
Copy link

fpietka commented Aug 3, 2022

We tested with & transformed to & and it wasn't valid with our bank. Which led me to think we would probably need to be able to do the basic conversion if needed.

@olleolleolle
Copy link
Contributor

Would it be interesting to be able to supply one's own set of replacements?

@fpietka
Copy link

fpietka commented Aug 5, 2022

@olleolleolle I'm divided about this, as I would rather not handle SEPA conversion in my own project 🤔.

FYI I was referring at the conversion table in which I think we can understand that basic character set often means removing them (which seems to be my case):
image

@kiriakosv
Copy link
Author

kiriakosv commented Aug 10, 2022

Hello, for special characters I followed chapter 6.2 here.

For the & case, the corresponding mapping is +. The only possibly problematic mappings are:

  • < to &lt
  • > to &gt
  • ' to &apos
  • "" to &quot

Two possible solutions that come to mind are:

  1. Replace these chars with an empty char (remove them)
  2. Raise error if at least one of this chars is present

What do you think?

@fpietka
Copy link

fpietka commented Sep 2, 2022

@kiriakosv what we're doing in the meantime is removing those chars.
Having an error would mean each user of the library would have to handle those themselves, which IMHO would not be desirable. Not sure having the option to be able catch an exception would bring much value, but I might be mistaken.

@kiriakosv
Copy link
Author

@kiriakosv what we're doing in the meantime is removing those chars.
Having an error would mean each user of the library would have to handle those themselves, which IMHO would not be desirable. Not sure having the option to be able catch an exception would bring much value, but I might be mistaken.

I too prefer removing these chars. If everyone is OK we can proceed like this.

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.

Extend allowed characters in InstanceMethods.convert_text
4 participants