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

feat(server,web) Semantic import path validation #7076

Merged
merged 35 commits into from Feb 20, 2024

Conversation

etnoy
Copy link
Contributor

@etnoy etnoy commented Feb 13, 2024

Check if import paths are valid and warn user if they aren't.

This should help some users to prevent simple mistakes I've seen several times in discord

  • Does the path exist?
  • Is the path a directory?
  • Do we have read access?

When scanning the library, import paths that do not pass validation are ignored

image

We also pop up a notification when the validation button is clicked
image

@etnoy etnoy added the external-library Issues related to external libraries label Feb 13, 2024
Copy link

cloudflare-pages bot commented Feb 13, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5b93c06
Status: ✅  Deploy successful!
Preview URL: https://87ba4580.immich.pages.dev
Branch Preview URL: https://fix-validate-import-path.immich.pages.dev

View logs

@jrasm91
Copy link
Contributor

jrasm91 commented Feb 13, 2024

Maybe the warning can go by the path itself not the action buttons

@etnoy
Copy link
Contributor Author

etnoy commented Feb 13, 2024

Maybe the warning can go by the path itself not the action buttons

Yes, the front end stuff needs more work, I'm currently focusing on the backend first

@etnoy etnoy marked this pull request as ready for review February 16, 2024 23:34
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.

Looking at all of this, is there any reason these checks can't be run inside of the library create and update methods? It is a bit confusing if you can create an invalid external library and have to validate it independently.

open-api/immich-openapi-specs.json Outdated Show resolved Hide resolved
@bo0tzz
Copy link
Member

bo0tzz commented Feb 17, 2024

IMO running the checks separately from changes makes sense. The library can break for external reasons (eg the mount goes offline), in which case having a check that immediately shows that would be nice.

@jrasm91
Copy link
Contributor

jrasm91 commented Feb 17, 2024

Sure, but that should be in addition to doing the check at time of creation imo.

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 pretty good!

server/src/domain/library/library.service.ts Outdated Show resolved Hide resolved
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.

LGTM

@alextran1502 alextran1502 merged commit b3c7beb into main Feb 20, 2024
25 checks passed
@alextran1502 alextran1502 deleted the fix/validate-import-path branch February 20, 2024 15:53
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.

None yet

4 participants