-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Exported this, so I could use it in the |
||
'alteration-ui-enabled': false, | ||
'banner-text': '', // by default, there is no banner text | ||
'change-ui-enabled': false, | ||
'conversion-ui-enabled': false, | ||
'restoration-ui-enabled': false, | ||
'sentry-enable': false, // by default, no sentry logs | ||
'supported-correction-entities': [] | ||
'supported-correction-entities': [], | ||
'supported-alteration-change-business-types': [] | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
/* This file is to provide the correct setup for the Vue instance. | ||
/* This file is to provide the correct setup for the Vue instance. Also for system wide mocks. | ||
* It can save people time when writing tests, as they wont need to figure out | ||
* why some of the errors are showing up due to Vue not having the plugins it needs. | ||
* See src/main.ts. | ||
|
@@ -10,6 +10,7 @@ import Affix from 'vue-affix' | |
import Vue2Filters from 'vue2-filters' // needed by SbcFeeSummary | ||
import Vuetify from 'vuetify' | ||
import { TiptapVuetifyPlugin } from 'tiptap-vuetify' | ||
import * as util from '@/utils/' | ||
|
||
Vue.use(Vuetify) | ||
Vue.use(Affix) | ||
|
@@ -28,3 +29,13 @@ Vue.use(TiptapVuetifyPlugin, { | |
// optional, default to 'md' (default vuetify icons before v2.0.0) | ||
iconsGroup: 'mdi' | ||
}) | ||
|
||
// 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. adding it to the list for next PR |
||
return ['BEN', 'BC', 'CC', 'ULC'] | ||
} else { | ||
return util.defaultFlagSet[name] | ||
} | ||
}) |
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).