15831 Replaced boolean correction flag with array flag #484
Conversation
- deleted old FF - added new FF - deleted old FF check - added new FF check
| 'restoration-ui-enabled': false, | ||
| 'sentry-enable': false, // by default, no sentry logs | ||
| 'banner-text': '' // by default, there is no banner text | ||
| 'supported-correction-entities': [] |
There was a problem hiding this comment.
In case LD can't be reached (esp in Prod), this is a safe fallback.
If you forget to add the dev LD key to your .env file, you get this value too 😁 Otherwise, your local and dev will have the same flags.
|
|
||
| /** Emits Have Data event. */ | ||
| @Emit('haveData') | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| private emitHaveData (haveData = true): void {} |
There was a problem hiding this comment.
I got rid of these because my TS/Vue extensions gives me errors that the template can't see these methods because they're private or protected (and yet it works). But it doesn't do this with all components and I don't know why...
Codecov Report
@@ Coverage Diff @@
## main #484 +/- ##
==========================================
+ Coverage 85.70% 85.72% +0.01%
==========================================
Files 177 195 +18
Lines 3289 3651 +362
Branches 524 445 -79
==========================================
+ Hits 2819 3130 +311
- Misses 468 508 +40
- Partials 2 13 +11
|
| // NB: specific entities are targeted via LaunchDarkly | ||
| if (!GetFeatureFlag('supported-correction-entities')?.includes(this.getEntityType)) { | ||
| window.alert('Corrections for this entity type are not available at the moment.\n' + | ||
| 'Please check again later.') | ||
| this.$root.$emit('go-to-dashboard', true) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
What do you think about adding the LD check to VueRouter as a guard using the route meta data to set the flag like this:
{
path: '/correction',
name: RouteNames.CORRECTION,
component: Correction,
meta: {
launchDarklyFlag: 'supported-correction-entities',
requiresAuth: true,
isStaffOnly: true,
filingType: FilingTypes.CORRECTION
}
}This approach would avoid duplicating the launch darkly check in each view and it also simplifies our testing since the view can be tested without having to write workaround for the launch darkly check.
There was a problem hiding this comment.
That's a very good idea and I was already thinking how to implement it, but this page/route doesn't know the entity type until it loads the draft filing.
Unless we change that.
There was a problem hiding this comment.
If we fetched the business info earlier then we would already know the entity type, but there is no "earlier" since this is the route we start on.
I'll keep thinking about this.
As for testing, it's actually easy to mock the feature flag call. See https://github.com/bcgov/business-filings-ui/blob/85c73c51a7db0eece1d4a2a8f097dca9a5f6fb5c/tests/unit/allowable-actions-mixin.spec.ts#L23
There was a problem hiding this comment.
Hmm, without putting logic in App.vue (to load the business info), I don't see how the router can do the FF check in this case. Do you have any ideas?
There was a problem hiding this comment.
Ah, good point. If I understand correctly, launch darkly returns an array of entity types when given a launch darkly flag -- right?
There was a problem hiding this comment.
Yes, the FF is an array of allowable entity types (for the given filing, eg, correction).
We could include entity type as an URL parameter. It seems a bit redundant since Edit UI can look it up but I do like the idea of the router being the "gatekeeper".
Note that we have different types of feature flags (eg, boolean, string, array), so the router would need to understand what each flag tests (ie, is specific to flag). What do you think about this?
There was a problem hiding this comment.
If you like the idea of router acting as a gatekeeper using FF, I suggest the cleanest way to implement this is to have the FF always return a Boolean. In edit-ui, it looks like all other feature flags (besides the one in this PR) return Booleans. If FF always return Booleans, it's easy to write a router guard that checks the appropriate FF. For corrections, you could use the same pattern by adding the filing type to the flag like this:
- supported-correction-entities-BEN-enabled
- supported-correction-entities-SP-enabled
- supported-correction-entities-GP-enabled
Not sure, the wording above is the best, but I hope it illustrates the point.
There was a problem hiding this comment.
It illustrates the point, but ouch. We have many filing-array flags, and I think it's desirable to keep the number of flags to a minimum (but let me check with Patrick).
Do you want any changes in this PR?
There was a problem hiding this comment.
Patrick says we can add dozens of new FF without impact to our LaunchDarkly plan.
But Thor says the number of flags matters (and it's unclear)... 🤷♂️
Future concern - new ticket.
There was a problem hiding this comment.
Great discussion Sev. I'll create a ticket to refactor the FF to use a VueRouter guard.
Issue #: bcgov/entity#15831
Description of changes:
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).