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

New match brand logic. #127

Merged
merged 1 commit into from
May 21, 2024
Merged

Conversation

pavelgyravel
Copy link
Contributor

@pavelgyravel pavelgyravel commented May 16, 2024

Вітаю!

Problem:

After adding new brand which intersects with existed brand prefix but with more accurate prefix, detector still return old brand value.

Example:

We have brand maestro with prefix 505. We add new brand kortimilli with more accurate prefix 505827028.
CreditCardValidations::Detector.add_brand(:kortimilli, length: 15, prefixes: '505827028')

But detector still return maestro instead kortimilli
CreditCardValidations::Detector.new('505827028341713').brand # => :maestro

Solution:
Instead of returning first matched by prefix brand, we get all matched and return one with more accurate prefix length.

@pavelgyravel
Copy link
Contributor Author

@Fivell, could you review please?

@Fivell
Copy link
Member

Fivell commented May 20, 2024

@pavelgyravel thanks will do soon

@Fivell
Copy link
Member

Fivell commented May 20, 2024

@pavelgyravel what do you think about different approach, just an idea, currently when we are adding branch rules in the beginning of the brands hash

https://github.com/didww/credit_card_validations/blob/v6.0.0/lib/credit_card_validations/detector.rb#L87

detecting mechanism find first key value pair and return brand

def valid_number?(*keys)
      selected_brands = keys.blank? ? self.brands : resolve_keys(*keys)
      if selected_brands.any?
        selected_brands.each do |key, brand|
          return key if matches_brand?(brand)
        end
      end
      nil
    end

what if we will try to insert new branch in the beginning so manually added rules will have better priority
something like this

instead of

   brands[key] = {rules: [], options: options || {}}

we can try something

  brand_config  = {key:  {rules: [], options: options || {}} } #define new hash for brand 
  brands = brand_config.merge(brands) #after this key : kortimilli will be in the beginning of the hash and 

after it brands keys will have koritmilli on the first place

tested with ruby2.7

 2.7.3 :001 > brands = {maestro: 'something', visa: 'something'}
 => {:maestro=>"something", :visa=>"something"} 
2.7.3 :002 > new_brand = {kortimilli: 'something' }
 => {:kortimilli=>"something"} 
2.7.3 :003 > brands = new_brand.merge(brands)
 => {:kortimilli=>"something", :maestro=>"something", :visa=>"something"} 
2.7.3 :004 > brands.keys
 => [:kortimilli, :maestro, :visa] 

end

if matched_brands.present?
return matched_brands.sort{|a, b| a[:matched_prefix_length] <=> b[:matched_prefix_length]}.last[:brand]
Copy link
Member

Choose a reason for hiding this comment

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

@pavelgyravel i think can be implemented in more easy way , commented PR with separate message take a look please

@pavelgyravel
Copy link
Contributor Author

@pavelgyravel what do you think about different approach, just an idea, currently when we are adding branch rules in the beginning of the brands hash

https://github.com/didww/credit_card_validations/blob/v6.0.0/lib/credit_card_validations/detector.rb#L87

detecting mechanism find first key value pair and return brand

def valid_number?(*keys)
      selected_brands = keys.blank? ? self.brands : resolve_keys(*keys)
      if selected_brands.any?
        selected_brands.each do |key, brand|
          return key if matches_brand?(brand)
        end
      end
      nil
    end

what if we will try to insert new branch in the beginning so manually added rules will have better priority something like this

instead of

   brands[key] = {rules: [], options: options || {}}

we can try something

  brand_config  = {key:  {rules: [], options: options || {}} } #define new hash for brand 
  brands = brand_config.merge(brands) #after this key : kortimilli will be in the beginning of the hash and 

after it brands keys will have koritmilli on the first place

tested with ruby2.7

 2.7.3 :001 > brands = {maestro: 'something', visa: 'something'}
 => {:maestro=>"something", :visa=>"something"} 
2.7.3 :002 > new_brand = {kortimilli: 'something' }
 => {:kortimilli=>"something"} 
2.7.3 :003 > brands = new_brand.merge(brands)
 => {:kortimilli=>"something", :maestro=>"something", :visa=>"something"} 
2.7.3 :004 > brands.keys
 => [:kortimilli, :maestro, :visa] 

@Fivell, thank you for your time!

Your proposal should work in my case. But in general, i think, algorithm I purposed, is more accurate. Frequently local brands has same prefix, but wider as brands like maestro, but thay are not kobrands or part of that brand. So I think that brand recognition wil better work when its based on more accurate intersection of prefix
For example:
505 -> maestro
505827001 -> kortimilli
5058270017 -> could be some other local brand
and in this case brand should be recognized propertly with no depending of order of adding to Detector.brands.

@Fivell
Copy link
Member

Fivell commented May 21, 2024

@pavelgyravel , ok i've got your point , lets try it !

@Fivell Fivell merged commit 6e86e81 into didww:master May 21, 2024
12 checks passed
@Fivell
Copy link
Member

Fivell commented May 21, 2024

@pavelgyravel https://rubygems.org/gems/credit_card_validations/versions/6.2.0

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.

2 participants