FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add test range param to NHS numbers by neanias · Pull Request #2947 · faker-ruby/faker · GitHub

/ faker Public

Add test range param to NHS numbers - #2947

Merged
stefannibrasil merged 1 commit into
faker-ruby:mainfrom
neanias:add-test-range-param-to-nhs-numbers
Jun 3, 2024
ConversationCommits (1)ChecksFiles changed
Merged

Add test range param to NHS numbers#2947
stefannibrasil merged 1 commit into
faker-ruby:mainfrom
neanias:add-test-range-param-to-nhs-numbers

Conversation

neanias commented Apr 29, 2024

Copy link
Copy Markdown
Contributor

The NHS sets aside a range of numbers for test purposes (999 000 0000 to 999 999 9999). By default, this generator creates valid NHS numbers that may be in use by actual people in the UK health services. This param will switch to using the test range which aren't in use anywhere.

Motivation / Background

This Pull Request has been created because the NHS sets aside a range of numbers for test purposes (999 000 0000 to 999 999 9999). By default, this generator creates valid NHS numbers that may be in use by actual people in the UK health services. This param will switch to using the test range which aren't in use anywhere.

Additional information

NHS Digital documentation about "synthetic" data

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug, refactor something, or add a feature.
  • Tests and Rubocop are passing before submitting your proposed changes.

thdaraujo reviewed May 7, 2024
View reviewed changes

thdaraujo left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Interesting, this makes sense to me. Maybe we should always use the test range (fake numbers) by default. We've done something similar to email addresses recently.

What do you think @stefannibrasil ?

Comment thread lib/faker/default/national_health_service.rb Outdated
# @faker.version 1.9.2
def british_number
base_number = rand(400_000_001...499_999_999)
def british_number(use_test_range: false)

thdaraujo May 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe we should always use a test range by default?

neanias May 7, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The reason why I put it as an opt-in was I was worried that it would break test suites if tests were checking that they were 4XX numbers rather than 999 numbers

thdaraujo May 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes, that makes sense too. Maybe the default could be flipped in a future release.

neanias May 7, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That was my thinking. Deprecate 4XX numbers and swap later

thdaraujo requested a review from stefannibrasil May 7, 2024 15:17
thdaraujo assigned thdaraujo and neanias and unassigned thdaraujo May 7, 2024
stefannibrasil reviewed May 8, 2024
View reviewed changes
Comment thread lib/faker/default/national_health_service.rb Outdated
def british_number
base_number = rand(400_000_001...499_999_999)
def british_number(use_test_range: false)
base_number = use_test_range ? rand(999_000_001...999_999_999) : rand(400_000_001...499_999_999)

stefannibrasil May 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What do you think of using a more readable Ruby alternative, i.e.:

Suggested change
base_number = use_test_range ? rand(999_000_001...999_999_999) : rand(400_000_001...499_999_999)
base_number = if use_test_range
rand(999_000_001...999_999_999)
else
rand(400_000_001...499_999_999)
end

stefannibrasil commented May 8, 2024

Copy link
Copy Markdown
Contributor

thank you @neanias this is good to know. As @thdaraujo mentioned, we did something similar for Faker URL and email addresses. I'm sure there are others but there's so many it's hard to spot all opportunities that need to be changed.

We can add a callout for this change when this PR gets released so folks know ahead of bumping the version. Therefore, I am okay with changing it to only generate test numbers by default and add the callout in the release notes.

Since we're working on this file, could you also change check_digit to be a private method?

neanias commented May 9, 2024

Copy link
Copy Markdown
Contributor Author

@stefannibrasil Ok, happy to just move it to be enabled by default! If we're changing the check_digit method to be private, should I also remove the related tests? The code paths will be exercised by the british_number method, I suppose.

stefannibrasil commented May 17, 2024

Copy link
Copy Markdown
Contributor

@stefannibrasil Ok, happy to just move it to be enabled by default! If we're changing the check_digit method to be private, should I also remove the related tests? The code paths will be exercised by the british_number method, I suppose.

Yes, that's the approach I follow too. No need to test private methods 👍

stefannibrasil reviewed May 28, 2024
View reviewed changes

stefannibrasil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looking good! Thank you for bringing this up. Could you also add a note in the docs for the new behavior?

https://github.com/faker-ruby/faker/blob/main/doc/default/national_health_service.md

Comment thread lib/faker/default/national_health_service.rb Outdated
neanias requested a review from stefannibrasil May 28, 2024 10:26
stefannibrasil reviewed May 28, 2024
View reviewed changes
Comment thread doc/default/national_health_service.md Outdated
Comment on lines +3 to +7
The NHS sets aside a range of numbers from 999 000 0000 to 999 999 9999 for
test purposes. This generator uses this range for creating NHS numbers rather
than the previous 400 000 0010 to 499 999 9999 range. The old range could
produce NHS numbers that were in use by real patients in the UK/England and
Wales.

stefannibrasil May 28, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
The NHS sets aside a range of numbers from 999 000 0000 to 999 999 9999 for
test purposes. This generator uses this range for creating NHS numbers rather
than the previous 400 000 0010 to 499 999 9999 range. The old range could
produce NHS numbers that were in use by real patients in the UK/England and
Wales.
The NHS sets aside a range of numbers from 999 000 0000 to 999 999 9999 for
test purposes. For more details, see [NHS Synthetic data](https://digital.nhs.uk/services/e-referral-service/document-library/synthetic-data-in-live-environments#synthetic-data-naming-convention).

We can leave the justification for the release notes and keep this directed to how it works 👍

Use test range to generate NHS numbers
28076c2 
The NHS sets aside a range of numbers for test purposes (999 000 0000 to
999 999 9999). Previously, this generator would generate valid NHS
numbers that may be in use by actual people in the UK health services.
This will now use the test range instead.
stefannibrasil approved these changes Jun 3, 2024
View reviewed changes

stefannibrasil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for bringing this up and improving Faker!

stefannibrasil merged commit f11e01a into faker-ruby:main Jun 3, 2024

neanias commented Jun 3, 2024

Copy link
Copy Markdown
Contributor Author

Glad we got it through in the end! 🚀

neanias deleted the add-test-range-param-to-nhs-numbers branch June 6, 2024 14:56

neanias commented Jul 1, 2024

Copy link
Copy Markdown
Contributor Author

Hello! Just wondering about this going in the recent changelog. It wasn't mentioned that it's potentially a breaking change for people using the generator and expecting the old range. Is it worth adding it later or is it just done for now?

stefannibrasil commented Jul 17, 2024

Copy link
Copy Markdown
Contributor

HI @neanias thanks for the reminder! I've cut a release with a note about this: https://github.com/faker-ruby/faker/releases/tag/v3.4.2

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

stefannibrasil stefannibrasil approved these changes
thdaraujo Awaiting requested review from thdaraujo

Assignees

neanias

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL