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.

Sign up for GitHub

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

Jump to bottom

plausible_number? is far too permissive #204

Open
asilano opened this issue Jan 28, 2021 · 6 comments
Open

plausible_number? is far too permissive #204

asilano opened this issue Jan 28, 2021 · 6 comments

Comments

@asilano
Copy link

asilano commented Jan 28, 2021

Because normalize_number strips all non-phonelike characters from its input, you get weird results like the following:

PhonyRails.plausible_number?('https://doi.org/10.1002/rse2.195', country_code: 'GB')
# => true
@joost
Copy link
Owner

joost commented Jan 28, 2021 via email

@asilano
Copy link
Author

asilano commented Jan 29, 2021

Well, it depends on what you, as owner, want the behaviour to be. I see that you strip any non-(digit, bracket or plus), and I can see that stripping dashes from +44-1234-567-890 (for instance) makes sense. Letters feel like they're less likely to appear in a valid number (especially since extensions are dealt with earlier).

Perhaps it should strip all non-phonelike, non-alphabetic characters?

@asilano
Copy link
Author

asilano commented Feb 19, 2021

@joost I want to try and submit a pull request, but I can't get tests running. I've:

  • Forked asilano/phony_rails
  • Cloned
  • Bundled
  • bundle exec rspec spec

I get the following error output:

No DRb server is running. Running in local process instead ...
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.

An error occurred while loading ./spec/lib/phony_rails_spec.rb.
Failure/Error: require 'mongoid'

NameError:
  uninitialized constant ActiveModel::Serializers::Xml
# ./spec/spec_helper.rb:12:in `require'
# ./spec/spec_helper.rb:12:in `<top (required)>'
# ./spec/lib/phony_rails_spec.rb:3:in `require'
# ./spec/lib/phony_rails_spec.rb:3:in `<top (required)>'
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.

An error occurred while loading ./spec/lib/validators/phony_validator_spec.rb.
Failure/Error: require 'mongoid'

NameError:
  uninitialized constant ActiveModel::Serializers::Xml
# ./spec/spec_helper.rb:12:in `require'
# ./spec/spec_helper.rb:12:in `<top (required)>'
# ./spec/lib/validators/phony_validator_spec.rb:3:in `require'
# ./spec/lib/validators/phony_validator_spec.rb:3:in `<top (required)>'
No examples found.

Finished in 0.00006 seconds (files took 1.1 seconds to load)
0 examples, 0 failures, 2 errors occurred outside of examples

[Coveralls] Outside the CI environment, not sending data.
My Gemfile.lock now looks like this: 
PATH
  remote: .
  specs:
    phony_rails (0.14.13)
      activesupport (>= 3.0)
      phony (> 2.15)

GEM
  remote: https://rubygems.org/
  specs:
    activemodel (6.1.3)
      activesupport (= 6.1.3)
    activerecord (6.1.3)
      activemodel (= 6.1.3)
      activesupport (= 6.1.3)
    activesupport (6.1.3)
      concurrent-ruby (~> 1.0, >= 1.0.2)
      i18n (>= 1.6, < 2)
      minitest (>= 5.1)
      tzinfo (~> 2.0)
      zeitwerk (~> 2.3)
    ast (2.4.2)
    bson (3.2.7)
    coderay (1.1.3)
    concurrent-ruby (1.1.8)
    connection_pool (2.2.3)
    coveralls (0.8.23)
      json (>= 1.8, < 3)
      simplecov (~> 0.16.1)
      term-ansicolor (~> 1.3)
      thor (>= 0.19.4, < 2.0)
      tins (~> 1.6)
    diff-lcs (1.4.4)
    docile (1.3.5)
    ffi (1.14.2)
    formatador (0.2.5)
    guard (2.16.2)
      formatador (>= 0.2.4)
      listen (>= 2.7, < 4.0)
      lumberjack (>= 1.0.12, < 2.0)
      nenv (~> 0.1)
      notiffany (~> 0.0)
      pry (>= 0.9.12)
      shellany (~> 0.0)
      thor (>= 0.18.1)
    guard-bundler (3.0.0)
      bundler (>= 2.1, < 3)
      guard (~> 2.2)
      guard-compat (~> 1.1)
    guard-compat (1.2.1)
    guard-rspec (4.7.3)
      guard (~> 2.1)
      guard-compat (~> 1.1)
      rspec (>= 2.99.0, < 4.0)
    i18n (1.8.9)
      concurrent-ruby (~> 1.0)
    json (2.5.1)
    listen (3.4.1)
      rb-fsevent (~> 0.10, >= 0.10.3)
      rb-inotify (~> 0.9, >= 0.9.10)
    lumberjack (1.2.8)
    method_source (1.0.0)
    minitest (5.14.3)
    mongoid (4.0.0.beta2)
      activemodel (>= 4.0.0)
      moped (~> 2.0.beta6)
      origin (~> 2.1)
      tzinfo (>= 0.3.37)
    moped (2.0.7)
      bson (~> 3.0)
      connection_pool (~> 2.0)
      optionable (~> 0.2.0)
    nenv (0.3.0)
    notiffany (0.1.3)
      nenv (~> 0.1)
      shellany (~> 0.0)
    optionable (0.2.0)
    origin (2.3.1)
    parallel (1.20.1)
    parser (3.0.0.0)
      ast (~> 2.4.1)
    phony (2.18.19)
    pry (0.14.0)
      coderay (~> 1.1)
      method_source (~> 1.0)
    rainbow (3.0.0)
    rake (13.0.3)
    rb-fsevent (0.10.4)
    rb-inotify (0.10.1)
      ffi (~> 1.0)
    regexp_parser (2.0.3)
    rexml (3.2.4)
    rspec (3.10.0)
      rspec-core (~> 3.10.0)
      rspec-expectations (~> 3.10.0)
      rspec-mocks (~> 3.10.0)
    rspec-core (3.10.1)
      rspec-support (~> 3.10.0)
    rspec-expectations (3.10.1)
      diff-lcs (>= 1.2.0, < 2.0)
      rspec-support (~> 3.10.0)
    rspec-mocks (3.10.2)
      diff-lcs (>= 1.2.0, < 2.0)
      rspec-support (~> 3.10.0)
    rspec-support (3.10.2)
    rubocop (1.10.0)
      parallel (~> 1.10)
      parser (>= 3.0.0.0)
      rainbow (>= 2.2.2, < 4.0)
      regexp_parser (>= 1.8, < 3.0)
      rexml
      rubocop-ast (>= 1.2.0, < 2.0)
      ruby-progressbar (~> 1.7)
      unicode-display_width (>= 1.4.0, < 3.0)
    rubocop-ast (1.4.1)
      parser (>= 2.7.1.5)
    rubocop-performance (1.9.2)
      rubocop (>= 0.90.0, < 2.0)
      rubocop-ast (>= 0.4.0)
    ruby-progressbar (1.11.0)
    shellany (0.0.1)
    simplecov (0.16.1)
      docile (~> 1.1)
      json (>= 1.8, < 3)
      simplecov-html (~> 0.10.0)
    simplecov-html (0.10.2)
    sqlite3 (1.4.2)
    sync (0.5.0)
    term-ansicolor (1.7.1)
      tins (~> 1.0)
    thor (1.1.0)
    tins (1.28.0)
      sync
    tzinfo (2.0.4)
      concurrent-ruby (~> 1.0)
    unicode-display_width (2.0.0)
    zeitwerk (2.4.2)

PLATFORMS
  ruby

DEPENDENCIES
  activerecord (>= 3.0)
  coveralls
  guard
  guard-bundler
  guard-rspec
  mongoid (>= 3.0)
  phony_rails!
  rake
  rspec
  rubocop
  rubocop-performance
  sqlite3 (>= 1.4.0)

BUNDLED WITH
   2.1.4

@joost
Copy link
Owner

joost commented Feb 19, 2021 •
edited
Loading

Hi @asilano! I see things broke when using phony_rails with the more recent gems.
I've fixed the tests by some updates & by removing MongoDb support.
I will push a branch for you to have a look at little later today.

@joost
Copy link
Owner

joost commented Feb 19, 2021

Please see the https://github.com/joost/phony_rails/tree/no_mongoid branch for a version with working specs. Pull request welcome!

@asilano
Copy link
Author

asilano commented Feb 22, 2021

Thanks, the branch works nicely.

Unfortunately, my fix doesn't - normalize_number is not massively to blame, and in fact Phony thinks that Phony.plausible?('https://example.com/15.55/a5b5c5/rse5.555') is true as well.

I'll have a think, and maybe I'll go bother them too :)

@asilano asilano mentioned this issue Feb 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@joost @asilano