Skip to content

clear collection titles before updating#280

Merged
elfkuzco merged 1 commit into
mainfrom
fix-update-title
May 18, 2026
Merged

clear collection titles before updating#280
elfkuzco merged 1 commit into
mainfrom
fix-update-title

Conversation

@elfkuzco
Copy link
Copy Markdown
Contributor

@elfkuzco elfkuzco commented May 12, 2026

Rationale

This PR clears the collection titles before creating the new ones to be used for an update. By clearing the old collection titles in memory, we avoid creating duplicate collection titles when the original collection title is sent in the update reqeust. This is because SQLAlchemy list of collection titles isn't cleared with just session.delete.
As a consequence, having duplicate collection titles leads to creating duplicate book locations which violates location unique constraint.

Changes

  • clear collection titles before adding new ones
  • update tests to model real-world scenario such that title to update has book, collection title, etc.
  • remove redundant second test since updated test covers everything

This fixes #277

@elfkuzco elfkuzco self-assigned this May 12, 2026
@elfkuzco elfkuzco requested a review from benoit74 May 12, 2026 09:39
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.37%. Comparing base (6408a8e) to head (c302d9b).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #280      +/-   ##
==========================================
+ Coverage   81.28%   81.37%   +0.09%     
==========================================
  Files          55       55              
  Lines        2362     2363       +1     
  Branches      236      236              
==========================================
+ Hits         1920     1923       +3     
+ Misses        381      380       -1     
+ Partials       61       60       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@elfkuzco
Copy link
Copy Markdown
Contributor Author

@benoit74 , I am wondering if this is not a good time to add a unique constraint to the collectiontitle table too. The keys for the constraint will be title_id, collection_id, and path. WDYT?

@benoit74
Copy link
Copy Markdown
Contributor

@benoit74 , I am wondering if this is not a good time to add a unique constraint to the collectiontitle table too. The keys for the constraint will be title_id, collection_id, and path. WDYT?

We already have a unique constraint (primary key actually) on title_id, collection_id for collection_title table, since this is more restrictive I don't think we need another one. Do I miss something?

Copy link
Copy Markdown
Contributor

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

LGTM

@elfkuzco
Copy link
Copy Markdown
Contributor Author

We already have a unique constraint (primary key actually) on title_id, collection_id for collection_title table,

No, we don't and we shouldn't because it acts as a many-to-many intermediary table for collection and title. But having a unique constraint made of title_id, collection_id and path will ensure we can't have the same title collection combination existing twice (at the DB level) although at the filesystem level, we intelligently avoid making duplicate copies on the same path.

@benoit74
Copy link
Copy Markdown
Contributor

But since title_id, collection_id is already unique, we are sure title_id, collection_id, path is unique as well. I don't get your point

@elfkuzco
Copy link
Copy Markdown
Contributor Author

But since title_id, collection_id is already unique, we are sure title_id, collection_id, path is unique as well. I don't get your point

Yup, indeed. Just realized they are both used as primary keys. Sorry, my bad.

@elfkuzco elfkuzco force-pushed the fix-update-title branch from 7f495bd to c302d9b Compare May 18, 2026 09:35
@elfkuzco elfkuzco merged commit e20f5fa into main May 18, 2026
2 of 3 checks passed
@elfkuzco elfkuzco deleted the fix-update-title branch May 18, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Impossible to add a second collection

2 participants