Hide power role for v3 apps #14813
Hide power role for v3 apps #14813
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
adrinr
changed the title
adrinr
requested review from
mike12345567 and
aptkingston
and removed request for
a team and
mike12345567
| * @deprecated the plan is to get everything using `tryGet` instead, then rename | ||
| * `tryGet` to `get`. | ||
| */ | ||
| export async function get() { |
There was a problem hiding this comment.
There is an application layer to the SDK - would it not be better to add this there?
There was a problem hiding this comment.
Not sure if that is exactly the same. I was actually doubting where to put that. I understood that the current application is for the crud of the apps itself, but this is for the app metadata specific. Do you think it's best to merge both?
There was a problem hiding this comment.
I think it probably makes sense to have metadata functions under the application, the CRUD of the application does fall under that part of the SDK, but the metadata is pretty much the only application document there is and I would default to looking for it under that part of the SDK.
Exactly. Having this new "creationVersion" will give us more flexibility. For example, it can be used in the future for certain migrations or similar, as we know when the app was created and what we don't need to support/migrate for that specific app |
| @@ -366,6 +383,18 @@ export async function getAllRoles(appId?: string): Promise<RoleDoc[]> { | |||
| } | |||
| } | |||
|
|
|||
| async function shouldIncludePowerRole(db: Database) { | |||
| const app = await db.tryGet<App>(DocumentType.APP_METADATA) | |||
| const creationVersion = app?.creationVersion | |||
There was a problem hiding this comment.
I'd be tempted to actually put this on the root Document type and call it _creationVersion, anticipating that we will likely introduce this idea onto other documents going forward.
There was a problem hiding this comment.
Yeah, then in DatabaseImpl.post we know when a doc is new because it has no ID, we could slap _creationVersion onto it there. Though it is a wide-reaching change that likely has risk.
There was a problem hiding this comment.
I'm unsure about this - I don't think we need this on every document, since every DB will have an app_metadata (it won't load without this/is corrupt) - in theory noting it here is enough since its a static document that is always easy to retrieve.
| } | ||
| ) | ||
|
|
||
| it.each(["2.9.0", "1.0.0"])( |
There was a problem hiding this comment.
I'd recommending including some of the other version strings we use, e.g. 0.0.0 (feature branches) and 2.32.17_2146.b125a7c (qa)
Description
Don't show POWER role for apps >= 3.x. This required adding a
createdVersionentry in the appmetadata, and to be able to run/test fine locally, we are updating the way we infer the current running version on dev, getting the value fromlerna.json(until now everything was set as 0.0.0)Launchcontrol
Do not show POWER role for apps >= 3.x