-
Notifications
You must be signed in to change notification settings - Fork 45
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
17178 - Alteration - Feature flag - Change Business Types #520
Conversation
Codecov Report
@@ Coverage Diff @@
## main #520 +/- ##
==========================================
- Coverage 87.11% 87.04% -0.07%
==========================================
Files 207 207
Lines 3919 3922 +3
Branches 510 510
==========================================
Hits 3414 3414
- Misses 491 494 +3
Partials 14 14
|
@@ -7,14 +7,15 @@ declare const window: any | |||
* Default flag values when LD is not available. | |||
* Uses "business-edit" project (per LD client id in config). | |||
*/ | |||
const defaultFlagSet: LDFlagSet = { | |||
export const defaultFlagSet: LDFlagSet = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exported this, so I could use it in the setup
// Mock feature flags for unit tests. | ||
jest.spyOn(util, 'GetFeatureFlag').mockImplementation( | ||
(name) => { | ||
if (name === 'supported-alteration-change-business-types') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is too global, let me know, I can call a function instead in the 2-3 sections that need it
I will create another PR and clean it up if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too global -- other features may want different feature flags.
Nice code, though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding it to the list for next PR
|
||
/** Called when component is mounted. */ | ||
mounted (): void { | ||
this.initializeEntityType() | ||
this.supportedEntityTypes = GetFeatureFlag('supported-alteration-change-business-types') || [] | ||
this.supportedEntityTypes = GetFeatureFlag('supported-alteration-change-business-types') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still fall back to []
in case LD returns null/undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahah I had a feeling - ok adding it to the list. We're going to have to make some more changes for the payment codes I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the conditional chaining on line 325 that you don't need if this variable is always an array (not falsy).
My guideline: don't use conditional chaining any more than you have to since it can hide errors (something that you'd want to catch during development or testing).
Issue #: /bcgov/entity#17178
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the bcrs-entities-create-ui license (Apache 2.0).