Add test range param to NHS numbers #2947
Conversation
There was a problem hiding this comment.
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 ?
| # | ||
| # @faker.version 1.9.2 | ||
| def british_number | ||
| base_number = rand(400_000_001...499_999_999) | ||
| def british_number(use_test_range: false) |
There was a problem hiding this comment.
Maybe we should always use a test range by default?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
yes, that makes sense too. Maybe the default could be flipped in a future release.
There was a problem hiding this comment.
That was my thinking. Deprecate 4XX numbers and swap later
| 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) |
There was a problem hiding this comment.
What do you think of using a more readable Ruby alternative, i.e.:
| 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 |
|
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 |
|
@stefannibrasil Ok, happy to just move it to be enabled by default! If we're changing the |
Yes, that's the approach I follow too. No need to test private methods 👍 |
There was a problem hiding this comment.
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
| 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. |
There was a problem hiding this comment.
| 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 👍
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.
|
Glad we got it through in the end! 🚀 |
|
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? |
|
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 |
A few months ago, we monkey patched our codebase (#1338) to work around the faker gem generating real NHS numbers. This has now been fixed upstream in faker (faker-ruby/faker#2947) and released in https://github.com/faker-ruby/faker/releases/tag/v3.4.2.
A few months ago, we monkey patched our codebase (#1338) to work around the faker gem generating real NHS numbers. This has now been fixed upstream in faker (faker-ruby/faker#2947) and released in https://github.com/faker-ruby/faker/releases/tag/v3.4.2.
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:
[Fix #issue-number]