Add browser-based folder import#17
Conversation
gosku
left a comment
There was a problem hiding this comment.
Thanks for this PR — the feature is shaping up well and the HTMX integration is clean. A few things to work through before it's ready to merge.
This review uses conventional comments to signal priority:
| Prefix | Blocking? | Meaning |
|---|---|---|
(convention) |
Yes | A public convention was violated |
(blocking) |
Yes | A functional issue that cannot be merged as-is |
(suggestion) |
No — reply sufficient | A private convention or quality issue |
(question) |
No — reply sufficient | Needs clarification |
There are inline comments attached to specific lines throughout the diff. Below are PR-level and design-level observations that apply more broadly.
Design
(suggestion) Consider replacing the typed-path input with a server-side folder browser.
Typing absolute paths is error-prone and unfamiliar to non-technical users. A clickable folder-tree picker — backed by a simple server-side endpoint that lists subdirectories at a given path — would be a meaningfully better UX for the same architectural cost. The browser makes AJAX calls as the user navigates, and the final selected path is sent as a plain string, which is exactly what this feature already expects.
This could be built as a small custom HTMX component (a directory listing partial that re-renders on each click), keeping the dependency footprint minimal. Dedicated libraries like django-filebrowser exist but are coupled to the Django admin interface and would not be appropriate here.
The autocomplete approach in this PR is a reasonable middle ground, but users would still need to know and type the start of their path.
PR-level
(convention) Commits fdcf72cd and 0b5a8012 have malformed subjects.
fdcf72cd:ui-feature: adding Import button to fetch images from a specific dire…— non-standard prefix, gerund mood instead of imperative ("adding" → "Add"), and the subject is truncated.0b5a8012:add directory auto-suggestion for imports— first word must be capitalised.
Subjects should be capitalised, imperative, and complete.
(convention) Commits 3–6 are unrebased fixup commits.
dce25982, 72869ebd, 11a77ab8, and 67edf2eb all correct things introduced in the first two commits. These should be rebased into the original commits before merge so the history reads as though everything was done right the first time.
72869ebd also violates atomicity on its own: it contains only test changes with no corresponding functional code. An atomic commit ships a feature and its tests together.
11a77ab8 introduces the InvalidFolderError-raising logic in _enqueue_images_in_folder and _process_images_in_folder with no unit tests. This is both an atomicity violation and a coverage gap — see the inline comment on process_images.py.
(convention) Commit fdcf72cd is not atomic — it ships production endpoints with no tests.
After fdcf72cd the codebase has two new URL routes and a view with no test coverage. Tests only arrive in the next commit. Each commit must leave the codebase in a deployable state.
| fujifilm_exif = models.FujifilmExif.get_or_create(**recipe_fields) | ||
| try: | ||
| fujifilm_exif = models.FujifilmExif.get_or_create(**recipe_fields) | ||
| except models.FujifilmExif.MultipleObjectsReturned: |
There was a problem hiding this comment.
(blocking) This fallback handler signals a missing UniqueConstraint on FujifilmExif.
MultipleObjectsReturned should never be raised by get_or_create if the model has a proper UniqueConstraint across its fields — the database would prevent duplicates from being created in the first place. Its presence here is a workaround for a missing constraint rather than correct design.
The correct fix requires a decision:
- Add a
UniqueConstraintacross allFujifilmExiffields — this makesget_or_createproperly atomic, makesMultipleObjectsReturnedimpossible by construction, and allows this fallback to be removed. - Accept duplicate rows — if identical combinations are harmless (e.g. they can only arise from reprocessing the same image), simplify to a plain
create()and drop the handler entirely.
Either way, this PR should not leave a silent race-condition workaround in the domain layer.
There was a problem hiding this comment.
Thanks for the clear breakdown.
Multiple images with identical recipe settings should resolve to the same row to keep imports consistent as duplicate rows would be noise to the app's core features.
However, there's a blocker on implementing option 1 on the UniqueConstraint across all FujifilmExif fields : PostgreSQL's B-tree indexes are limited to 32 columns (source https://www.postgresql.org/docs/current/indexes-multicolumn.html), and a UniqueConstraint is backed by a unique index. FujifilmExif has more than 32 recipe-defining fields, so a straight UniqueConstraint(fields=[...]) across all of them won't build on Postgres.
Here are few options :
-
Hash field. Add a
CharField(unique=True)storing a deterministic hash (e.g. SHA-256) of the recipe fields, computed insave()or via a pre-save signal.get_or_createlooks up by hash. Works on any backend, constraint is single-column. Downside: the uniqueness key is opaque in the DB, and the migration needs a data backfill for existing rows. -
Constraint over a curated identity subset. Pick the fields that define a recipe's identity (like film simulation, grain, WB, DR, tone curve, colour settings, etc.) and put the
UniqueConstraintover those only, staying under 32. This option is transparent in the DB. Downside: requires deciding which fields count as "identity" vs. which are metadata.
Happy to go with whichever you prefer, please let me know if you think about any alternative.
There was a problem hiding this comment.
I think the problem is that FujifilmExif is a FK when it should be a OneToOneField. Given one Image, the possibility of having a duplicate FujifilmExif is minimal. This would only happen if the same Image is already in DB and we are trying to process it again. Therefore, I believe that FujifilmExif should invoke a .create() method instead. Ideally, the entire operation should be wrapped in an atomic transaction so, if we try to create an Image that is already there, we could roll back the FujifilmExif creation, but for the moment I can live with having orphan duplicated FujifilmExif objects in DB.
As a summary, we shouldn't have to handle MultipleObjectsReturned exceptions.
There was a problem hiding this comment.
Gone with proposition 2. I replaced get_or_create with a FujifilmExif.create(). Concerning the FK to OneToOneField, I assume it should be addressed out of this PR.
f517125 to
42d0222
Compare
42d0222 to
b0f0057
Compare
b0f0057 to
5eaa925
Compare
5eaa925 to
d12c573
Compare
d12c573 to
e0710ae
Compare
|
I rebased history into 2 atomic commits + 1 commit for convention fixes. Also added a UI harmonization commit aligning the 'Images' page gallery with the 'Recipes' page card style. |
Until now, importing images requires running command from terminal. This PR adds a browser-based alternative: a folder path input directly in the gallery, so users can trigger an import without leaving the UI. No files are copied, the app reads metadata from wherever the images already live on disk.
What Changed
UI (
gallery.html)New "Import" button opens an inline form.
Autocomplete (
GET /images/import/suggest/)As the user types a path, matching subdirectories are returned as an HTML list. Hidden directories are excluded, results are capped at 15.
Import endpoint (
POST /images/import/)Validates the supplied path, calls the use case, and returns an HTMX partial reporting how many images were imported. Non-HTMX requests redirect to the gallery. Invalid paths return a user-facing error message.
Use cases (
src/application/usecases/images/process_images.py)import_images_from_folder()wrapper around the existing sync and async paths. RaisesInvalidFolderErrorfor non-existent or non-directory paths.suggest_import_folder()delegates to the domain query and keeps views from calling the domain layer directly.Domain (
src/domain/images/queries.py)suggest_subdirectories()resolves partial path input into a sorted list of matchingPathobjects usingos.scandirHow to Test
Tests
tests/functional/test_import_folder_view.pytests/functional/test_import_folder_suggest_view.py