Feature: Read-only property for External Libraries #8992
Conversation
|
Thanks, that's what I need most about this feature, otherwise I wouldn't even know how to do my job! |
|
Thanks! I discussed this PR with the other maintainers and we're happy with it overall, but adding that additional server-side check would be a blocker. Then I think the only other change needed would be some updates to the tests
I believe we already have some indications such as delete being disabled for readonly assets
Probably better to make the readonly property modifiable in the UI rather than a cmdline tool, but that's probably better as a separate PR. I think this is fine for now |
|
Thanks for keeping an eye on this feature, we're excited to see it coming soon! |
|
#8943 seems more complete |
|
@jrasm91 in what way does it seem more complete? |
|
Both look to be doing essentially the same thing. The other PR was created first, has some tests, and allows the property to be updated after the library is created. |
|
@benmccann |
rokiden
force-pushed
the
feat/del-from-ext-lib
branch
from
1d2d83e
to
9e87ea2
Compare
| @@ -252,11 +252,19 @@ export class AssetService { | |||
| async update(auth: AuthDto, id: string, dto: UpdateAssetDto): Promise<AssetResponseDto> { | |||
| await this.access.requirePermission(auth, Permission.ASSET_UPDATE, id); | |||
|
|
|||
| let asset = await this.assetRepository.getById(id); | |||
There was a problem hiding this comment.
this is going to cause a second query right after having done a query on the line above. Perhaps it should just be part of that initial permission check. see AssetAcess.checkOwnerAccess in access.repository.ts
There was a problem hiding this comment.
That would be fine with me. Asset update access implies the asset is not read only.
| @@ -37,6 +39,7 @@ | |||
| <p class="p-5 text-sm">NOTE: This cannot be changed later!</p> | |||
There was a problem hiding this comment.
Assuming #9188 is merged, then you will be able to change the readonly status, so we might need to clarify what "This" is referring to in this message
|
Hey, thanks for working on this and sorry for the confusion/back and forth around this topic. Internally we have had a lot of discussions about the direction we want to take external libraries. Long story short, we decided to go ahead with the implementation laid out in #9235. |
|
This is good news, for me it’s much better that the team took it upon themselves. Thanks! I was glad to help. |
Closes #5449. Original thread .
This PR adds "Read Only" property for External library.
In database it stored as column "libraries.isReadOnly". This property set at library creation time and can't be modified. Asset.isReadOnly inherits library's value at creation time. At frontend it was added as toggle to "Select library owner" dialog. Mobile app doesn't require changes.
Asset modification and deletion require to check only isReadOnly property, isExternal becomes only informational.
Tested:
TODO: