Skip to content
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

Option to disable readonly status of external libraries #9188

Closed
wants to merge 24 commits into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Apr 30, 2024

This is an updated version of @valpackett's #8943 with the code review comments addressed

closes #8943
closes #5449
ref #8438

@benmccann benmccann added the external-library Issues related to external libraries label Apr 30, 2024
@mertalev
Copy link
Contributor

I see the library's isReadOnly getting updated, but don't see where that changes the asset isReadOnly. That would have to either be a separate query or a DB trigger. The library's value would also need to be applied to assets that get added afterwards.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are still missing the server implementation:

  1. to set the value when assets are created (via a library scan)
  2. to flip the flag on affected assets when the library is updated.

server/src/services/asset.service.ts Outdated Show resolved Hide resolved
web/src/routes/admin/library-management/+page.svelte Outdated Show resolved Hide resolved
@benmccann benmccann changed the title Allow option to disable readonly status of external libraries Option to disable readonly status of external libraries May 1, 2024
@benmccann
Copy link
Contributor Author

Thanks @mertalev and @jrasm91 for pointing out that the original PR didn't actually flip the relevant flag. I've updated that now

@@ -201,6 +201,10 @@ export class AssetRepository implements IAssetRepository {
);
}

async setReadOnlyForLibrary(libraryId: string, isReadOnly: boolean): Promise<void> {
await this.repository.update({ library: { id: libraryId } }, { isReadOnly });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External libraries can have isVisible:false motion parts of extracted live photos and they need to be excluded from this update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. I've updated it, but you should check that I understood correctly: 3e19aef

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that looks good. Will have to test later with motion photos to make sure are working as expected.

@jrasm91
Copy link
Contributor

jrasm91 commented May 3, 2024

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.

@jrasm91 jrasm91 closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-library Issues related to external libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants