[AC-1970] Add billing navigation group to provider layout #8941
Conversation
…n page behind FF.
|
No New Or Fixed Issues Found |
| import { UserId } from "../../types/guid"; | ||
| import { ProviderData } from "../models/data/provider.data"; | ||
| import { Provider } from "../models/domain/provider"; | ||
|
|
||
| export abstract class ProviderService { | ||
| hasConsolidatedBilling$: (id: string) => Observable<boolean>; |
There was a problem hiding this comment.
This observable is less about moving providers to and from state, which is the scope we are trying to keep services at in this level of on admin console code, and is more about applying business logic to a returned observable to filter out unwanted data.
Would you be up for exporting this as a mapping function instead? You can find some examples of how we are doing this in organizationService here. It could be exported from this file, or I'd even say it might be better to export it from some place owned by billing so you don't have to worry about us needing to review changes.
Creating walled gardens between AC and Billing seems like a challenge, so anything we can do to isolate I think is useful to do even if just for that reason.
That said: this particular example has some variables in it that make me wonder how well it'll translate to an essentially static function. Using a feature flag is one example. If you notice or feel like these things stack up in a way that make it not worth it, just let me know.
There was a problem hiding this comment.
Great suggestion! I've moved this to the provider-billing.service.abstraction file and called it canAccessBilling. It takes the configService dependency as a parameter rather than an injection since I don't actually need a full class yet. Changes here: 4d5dbb1#diff-9ebdce965c4d4d8fbb70b0ae033fdce9629dea1a7c4b16dfb40df8c8d03cb6f9
| }); | ||
| }); | ||
|
|
||
| describe("get$()", () => { |
| const configService = inject(ConfigService); | ||
| const providerService = inject(ProviderService); | ||
|
|
||
| const provider = await providerService.get(route.params.providerId); |
There was a problem hiding this comment.
Would it be possible to use the observable get$ here instead? You'll probably still have to await it since this is a guard, and that's fine, but we are planning to remove the promise based methods on providerService soon.
There was a problem hiding this comment.
Yeah because I'm asyncing everything in this guard I just did firstValueFrom out of the get$ observable. Changes here: 4d5dbb1#diff-9ebdce965c4d4d8fbb70b0ae033fdce9629dea1a7c4b16dfb40df8c8d03cb6f9
| @@ -104,6 +89,10 @@ export class ManageClientOrganizationsComponent extends BaseClientsComponent { | |||
| } | |||
|
|
|||
| async load() { | |||
| this.provider = await this.providerService.get(this.providerId); | |||
There was a problem hiding this comment.
Can you use the observable get$ here instead? We're planning to remove the promise based methods from AC's domain services soon.
There was a problem hiding this comment.
Yes! Missed this one initially but added it here: d8339ac
Type of change
Objective
This PR adds the Billing navigation group to the provider layout when the FF
enable-consolidated-billingis on and the provider'sproviderStatusisBillable.It also does some minor refactoring on the fly.
Code changes
get$andhasConsolidatedBilling$; the latter using the feature flag and the provider state to return an observable.hasConsolidatedBilling$istrue.hasConsolidatedBilling$method.hasConsolidateBilling$method./providers/{providerId}when CB is not active for the provider.Screenshots
Screen.Recording.2024-04-26.at.1.41.06.PM.mov