feat: Add purchase date tracking for books#2673
feat: Add purchase date tracking for books#2673pratikscfr wants to merge 17 commits intobooklore-app:developfrom
Conversation
- Add purchase_date column via V117 migration - Update BookEntity and Book DTO - Add updatePurchaseDate endpoint in BookController - Implement update logic in BookUpdateService - Update mappers and creator services to handle new field
- Add test cases for authorized purchase date updates - Add test cases for unauthorized access attempts - Add test cases for fallback to addedOn date - Add test cases for missing book scenarios
- Add purchaseDate field to Book interface - Update BookPatchService to handle purchase date updates - Update sorting logic to support sorting by purchase date
- Add Purchase Date column to book table - Add editable Purchase Date field in metadata viewer - Update Reading Journey and Backlog charts to use purchase date (falling back to added date)
- Add BookPatchService tests for purchase date updates - Test successful update scenarios - Test fallback to addedOn date logic
- Restrict p-datepicker maxDate to current date - Initialize maxDate in component
- Add validation check in BookUpdateService - Throw INVALID_INPUT error if date is future - Add unit test for future date validation
|
This PR addresses : #2496 |
|
Hello @acx10 / @balazs-szucs , can you review this PR? |
| .libraryPath(libraryFile.getLibraryPathEntity()) | ||
| .addedOn(Instant.now()) | ||
| .addedOn(now) | ||
| .purchaseDate(now) |
There was a problem hiding this comment.
We should not automatically set purchaseDate to now(). Adding a book to the library does not imply it was purchased at that time. This field should remain null unless explicitly provided.
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE book ADD COLUMN purchase_date DATETIME(6) NULL; | |||
There was a problem hiding this comment.
ALTER TABLE book ADD COLUMN IF NOT EXISTS purchase_date DATETIME(6) NULL;
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE book ADD COLUMN purchase_date DATETIME(6) NULL; | |||
| UPDATE book SET purchase_date = added_on WHERE purchase_date IS NULL; | |||
There was a problem hiding this comment.
This backfill isn’t needed. purchaseDate should remain NULL unless explicitly set.
| // Track added dates | ||
| if (book.addedOn) { | ||
| const monthKey = this.getMonthKey(book.addedOn); | ||
| const acquisitionDate = book.purchaseDate || book.addedOn; |
There was a problem hiding this comment.
purchaseDate is not needed in any of the current charts.
There was a problem hiding this comment.
Pull request overview
Adds end-to-end “purchase date” support for books across the API and UI, including persistence, update endpoint, and UI display/sorting/stats integration.
Changes:
- Add
purchase_dateto the DB + backend entity/DTO mapping and initialize it for new books. - Add an API endpoint to update purchase dates with authorization and future-date validation.
- Update UI models, metadata viewer editing, sorting options, and stats charts to use purchase date (with added-on fallback in several places), plus add Vitest/JUnit coverage.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| booklore-api/src/main/resources/db/migration/V117__Add_purchase_date_to_book.sql | Adds purchase_date column and backfills it from added_on. |
| booklore-api/src/main/java/org/booklore/model/entity/BookEntity.java | Adds purchaseDate field to the JPA entity. |
| booklore-api/src/main/java/org/booklore/model/dto/Book.java | Exposes purchaseDate on the API DTO. |
| booklore-api/src/main/java/org/booklore/mapper/BookMapper.java | Maps purchaseDate from entity to DTO. |
| booklore-api/src/main/java/org/booklore/model/dto/request/PurchaseDateUpdateRequest.java | Adds request type for purchase-date updates. |
| booklore-api/src/main/java/org/booklore/controller/BookController.java | Adds PUT /api/v1/books/purchase-date endpoint with authorization. |
| booklore-api/src/main/java/org/booklore/service/book/BookUpdateService.java | Implements purchase-date update logic (authorization + future date validation). |
| booklore-api/src/main/java/org/booklore/service/book/BookCreatorService.java | Sets purchase date when creating shell/imported books. |
| booklore-api/src/main/java/org/booklore/service/book/PhysicalBookService.java | Sets purchase date when creating physical books. |
| booklore-api/src/test/java/org/booklore/service/book/BookUpdateServiceTest.java | Adds JUnit coverage for purchase-date update scenarios. |
| booklore-ui/src/app/features/book/model/book.model.ts | Adds purchaseDate?: string to the UI Book model. |
| booklore-ui/src/app/features/book/service/book.service.ts | Adds UI service wrapper updatePurchaseDate(...). |
| booklore-ui/src/app/features/book/service/book-patch.service.ts | Implements HTTP call + local state update for purchase date changes. |
| booklore-ui/src/app/features/book/service/book-patch.service.spec.ts | Adds Vitest coverage for purchase-date patching/state updates. |
| booklore-ui/src/app/features/book/service/sort.service.ts | Adds purchase-date sort field extractor. |
| booklore-ui/src/app/features/book/service/sort.service.spec.ts | Adds Vitest coverage for purchase-date sorting. |
| booklore-ui/src/app/features/book/components/book-browser/sorting/BookSorter.ts | Adds “Purchase Date” to selectable sort options. |
| booklore-ui/src/app/features/book/components/book-browser/book-table/book-table.component.ts | Adds “Purchased” column rendering for purchase date. |
| booklore-ui/src/app/features/metadata/component/book-metadata-center/metadata-viewer/metadata-viewer.component.ts | Adds purchase-date editing state + save/cancel handlers. |
| booklore-ui/src/app/features/metadata/component/book-metadata-center/metadata-viewer/metadata-viewer.component.html | Adds purchase-date display + PrimeNG date picker editing UI. |
| booklore-ui/src/app/features/stats/component/user-stats/charts/reading-backlog-chart/reading-backlog-chart.component.ts | Uses purchase date (fallback to added-on) in backlog calculations. |
| booklore-ui/src/app/features/stats/component/library-stats/charts/reading-journey-chart/reading-journey-chart.component.ts | Uses purchase date (fallback to added-on) in journey stats/insights. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| addedOn: (book) => book.addedOn ? new Date(book.addedOn).getTime() : null, | ||
| purchaseDate: (book) => book.purchaseDate ? new Date(book.purchaseDate).getTime() : null, | ||
| fileSizeKb: (book) => book.fileSizeKb || null, |
There was a problem hiding this comment.
The new purchaseDate sort extractor ignores the stated fallback to addedOn that other parts of the UI use (charts/metadata viewer use purchaseDate || addedOn). If purchaseDate can be missing, sorting by it will push those books to the end instead of sorting by their acquisition date. Consider extracting from purchaseDate ?? addedOn for consistency.
| case 'purchaseDate': | ||
| return book.purchaseDate ? this.datePipe.transform(book.purchaseDate, 'dd-MMM-yyyy') ?? '' : ''; | ||
|
|
There was a problem hiding this comment.
The new Purchased column renders an empty string when purchaseDate is missing. Elsewhere (metadata viewer, stats) you fall back to addedOn. If the intended behavior is to always show an acquisition date, consider rendering purchaseDate ?? addedOn here as well.
| BookLoreUser user = authenticationService.getAuthenticatedUser(); | ||
| if (user == null) { | ||
| throw ApiError.FORBIDDEN.createException("You are not authorized to update purchase dates."); | ||
| } |
There was a problem hiding this comment.
updatePurchaseDate calls bookRepository.findAllById(bookIds) before validating bookIds. If the request body omits bookIds (null) this will throw a server-side exception (500) instead of a controlled 4xx. Add an early guard for bookIds == null || bookIds.isEmpty() (and ideally return a BAD_REQUEST / INVALID_INPUT error) before the repository call.
| } | |
| } | |
| if (bookIds == null || bookIds.isEmpty()) { | |
| throw ApiError.INVALID_INPUT.createException("Book IDs must be provided."); | |
| } |
| import java.time.Instant; | ||
| import java.util.List; | ||
|
|
||
| public record PurchaseDateUpdateRequest(List<Long> bookIds, Instant purchaseDate) { |
There was a problem hiding this comment.
PurchaseDateUpdateRequest is annotated with @Valid at the controller, but the record components have no bean validation constraints. Consider adding at least @NotEmpty/@NotNull on bookIds so invalid payloads fail fast with a 400 rather than propagating deeper into the service layer.
| public record PurchaseDateUpdateRequest(List<Long> bookIds, Instant purchaseDate) { | |
| import jakarta.validation.constraints.NotEmpty; | |
| import jakarta.validation.constraints.NotNull; | |
| public record PurchaseDateUpdateRequest( | |
| @NotEmpty List<Long> bookIds, | |
| @NotNull Instant purchaseDate | |
| ) { |
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE book ADD COLUMN purchase_date DATETIME(6) NULL; | |||
There was a problem hiding this comment.
The migration adds purchase_date as DATETIME(6), while the existing book.added_on column is TIMESTAMP (see V1__Create_Tables.sql). Mixing DATETIME and TIMESTAMP for the same logical timestamp type can introduce subtle timezone/default/precision inconsistencies. Consider using the same column type/precision as added_on (e.g., TIMESTAMP(6) if supported) or document why DATETIME(6) is required here.
| ALTER TABLE book ADD COLUMN purchase_date DATETIME(6) NULL; | |
| ALTER TABLE book ADD COLUMN purchase_date TIMESTAMP(6) NULL; |
| private getBooksWithAddedDate(books: Book[]): Book[] { | ||
| return books.filter(book => book.addedOn); | ||
| return books.filter(book => book.purchaseDate || book.addedOn); | ||
| } |
There was a problem hiding this comment.
getBooksWithAddedDate now filters on purchaseDate || addedOn, so the method name is no longer accurate and can mislead future readers. Rename it (and the booksWithDates variable) to reflect an acquisition date (purchase-or-added) concept.
| private calculateInsights(books: Book[], monthlyData: MonthlyData[]): JourneyInsights { | ||
| const booksWithAddedDate = books.filter(b => b.addedOn); | ||
| const booksWithAddedDate = books.filter(b => b.purchaseDate || b.addedOn); | ||
| const booksFinished = books.filter(b => b.dateFinished && b.readStatus === ReadStatus.READ); |
There was a problem hiding this comment.
booksWithAddedDate now includes books with purchaseDate, so the variable name (and totalAdded meaning) is misleading. Consider renaming to something like booksWithAcquisitionDate / totalAcquired to match the updated logic and avoid confusion when interpreting the insights.
| throw ApiError.GENERIC_NOT_FOUND.createException("One or more books not found"); | ||
| } | ||
|
|
||
| boolean isAdmin = user != null && user.getPermissions() != null && user.getPermissions().isAdmin(); |
There was a problem hiding this comment.
This check is useless. user cannot be null at this check, since it is guarded by ... == ....
| boolean isAdmin = user != null && user.getPermissions() != null && user.getPermissions().isAdmin(); | |
| boolean isAdmin = user.getPermissions() != null && user.getPermissions().isAdmin(); |
|
Thanks @acx10 for the review. I will look into them |
|
Hello @acx10 , I made the changes requested by you. Can you take a look now? |
|
Hi @acx10 , any more changes needed for this? |
|
Hi, can you checkout if the field is written each file type e.g., PDFs EPUBs, and a CBZ? When the option enabled to "write to file" |
Added the support for adding purchase date meta data when enabled for types : epub, pdf, cbz |
|
The primary reason for writing metadata to file is persistence. Simply writing to the metadata container is insufficient; you must validate the parser and do end-to-end testing. For example, if you write metadata to a file, delete the library, and cannot retrieve the same metadata when recreating the library, this indicates a problem. Additionally, your current implementation does not conform to the Anansi Project's schema for CBX files: https://anansi-project.github.io/docs/comicinfo/schemas/v2.0 While writing metadata to the file is the straightforward component, please update the implementation to address these issues and ensure end-to-end testing is performed. |
|
Hi @balazs-szucs , added your requested changes. |
|
Hi @balazs-szucs / @acx10 , can I have this reviewed, thanks? |
| @Mock | ||
| private ContentRestrictionService contentRestrictionService; | ||
| @Mock | ||
| private org.booklore.service.metadata.writer.MetadataWriterFactory metadataWriterFactory; |
There was a problem hiding this comment.
The new mock fields use fully qualified class names (FQN) instead of proper imports. This violates the project style. Please add proper import statements and use short names:
@Mock
private MetadataWriterFactory metadataWriterFactory;
@Mock
private AppSettingService appSettingService;
@Mock
private SidecarMetadataWriter sidecarMetadataWriter;Same issue applies throughout the setUp() method where org.booklore.model.dto.settings.MetadataPersistenceSettings.SaveToOriginalFile, org.booklore.model.dto.settings.MetadataPersistenceSettings, and org.booklore.model.dto.settings.AppSettings are used as FQNs.
| try { | ||
| builder.purchaseDate(java.time.Instant.parse(purchaseDateStr.trim())); | ||
| } catch (Exception ignored) { | ||
| } |
There was a problem hiding this comment.
FQN usage here too: java.time.Instant.parse(purchaseDateStr.trim()). Please add import java.time.Instant; at the top and use Instant.parse(...).
Same issue in EpubMetadataExtractor.java and PdfMetadataExtractor.java with java.time.Instant.parse(content).
| boolean isAdmin = user.getPermissions() != null && user.getPermissions().isAdmin(); | ||
| if (!isAdmin) { | ||
| Set<Long> allowedLibraryIds = user.getAssignedLibraries().stream().map(Library::getId).collect(Collectors.toSet()); | ||
| boolean hasAnyForbiddenLibrary = books.stream().anyMatch(b -> b.getLibrary() == null || !allowedLibraryIds.contains(b.getLibrary().getId())); |
There was a problem hiding this comment.
java.io.File is used as FQN here. Please import it and use File.
| @@ -359,6 +370,84 @@ private List<PersonalRatingUpdateResponse> buildRatingUpdateResponses(List<Long> | |||
| .collect(Collectors.toList()); | |||
There was a problem hiding this comment.
Missing audit logging. The project convention requires auditService.log(...) calls for create/update/delete operations. Other update methods in this service use audit logging for significant state changes. Consider adding something like:
auditService.log(AuditAction.METADATA_UPDATED, "Updated purchase date for book(s): " + bookIds);
| @Mapping(source = "libraryPath", target = "libraryPath", qualifiedByName = "mapLibraryPathIdOnly") | ||
| @Mapping(source = "metadata", target = "metadata") | ||
| @Mapping(source = "shelves", target = "shelves") | ||
| @Mapping(source = "bookFiles", target = "primaryFile", qualifiedByName = "mapPrimaryFile") |
There was a problem hiding this comment.
This @Mapping is redundant. When the source and target field names are identical, MapStruct maps them automatically. You can safely remove both of these purchaseDate mapping lines.
| () -> bookUpdateService.updatePurchaseDate(List.of(1L), Instant.now())); | ||
| assertTrue(ex.getMessage().toLowerCase().contains("not authorized")); | ||
| verify(bookRepository, never()).saveAll(any()); | ||
| } |
There was a problem hiding this comment.
The test name says "shouldFallbackToAddedOnWhenNull" but the backend doesn't actually fall back to addedOn. It just sets purchaseDate to null and the test asserts assertNull(book.getPurchaseDate()). The "fallback to addedOn" logic only exists in the frontend for display purposes.
Suggest renaming to something like updatePurchaseDate_shouldSetNullWhenPurchaseDateIsNull to accurately reflect the behavior being tested.
| @@ -31,6 +31,7 @@ public class BookMetadata { | |||
| private String language; | |||
| private String narrator; | |||
| private Boolean abridged; | |||
There was a problem hiding this comment.
purchaseDate is already on Book.java DTO (which is the book-level field). Adding it also to BookMetadata DTO seems redundant since purchase date is stored on BookEntity, not BookMetadataEntity. This dual placement could be confusing for future developers.
Consider removing it from BookMetadata and only keeping it on Book. The metadata viewer already accesses it via book.purchaseDate.
| if (!isAdmin) { | ||
| Set<Long> allowedLibraryIds = user.getAssignedLibraries().stream().map(Library::getId).collect(Collectors.toSet()); | ||
| boolean hasAnyForbiddenLibrary = books.stream().anyMatch(b -> b.getLibrary() == null || !allowedLibraryIds.contains(b.getLibrary().getId())); | ||
| if (hasAnyForbiddenLibrary) { |
There was a problem hiding this comment.
This method calls writer.saveMetadataToFile(file, book.getMetadata(), null, new MetadataClearFlags()) which writes all metadata to the file, not just the purchase date. If the user only updated the purchase date, this will also re-serialize every other metadata field.
This could cause issues if:
- The metadata entity isn't fully loaded (lazy loading)
- Another concurrent operation modified metadata
Is this intentional, or should this use a more targeted approach similar to how other single-field updates work?
|
|
||
| public void copyPurchaseDate(boolean clear, Consumer<Instant> consumer) { | ||
| if (clear) consumer.accept(null); | ||
| else if (metadata.getBook() != null && metadata.getBook().getPurchaseDate() != null) |
There was a problem hiding this comment.
Minor style nit: the else-if body is missing braces, which is inconsistent with the rest of the methods in this class. Other methods like `copyPublishedDate` use braces:
```java
public void copyPurchaseDate(boolean clear, Consumer consumer) {
if (clear) {
consumer.accept(null);
} else if (metadata.getBook() != null && metadata.getBook().getPurchaseDate() != null) {
consumer.accept(metadata.getBook().getPurchaseDate());
}
}
```
Also, unlike other `copy*` methods, this one doesn't check a lock status. That seems intentional since purchase date lives on `BookEntity` rather than `BookMetadataEntity`, but it's worth calling out for clarity.
| if (user == null) { | ||
| throw ApiError.FORBIDDEN.createException("You are not authorized to update purchase dates."); | ||
| } | ||
| List<BookEntity> books = bookRepository.findAllById(bookIds); |
There was a problem hiding this comment.
The null check bookIds != null on this line is redundant since you already checked bookIds == null above and would have thrown before reaching here.
| books.forEach(book => { | ||
| const addedDate = new Date(book.addedOn!); | ||
| if (!book.addedOn) { | ||
| return; |
There was a problem hiding this comment.
The `reading-backlog-chart` null guard and the `let` to `const` changes in `book.service.ts` and `sort.service.ts` are unrelated to purchase date tracking. While individually fine, mixing unrelated cleanups into a feature PR makes the diff harder to review and bisect.
Consider keeping these as a separate commit or a small cleanup PR.
| helper.copyPurchaseDate(clear != null && clear.isPurchaseDate(), date -> { | ||
| if (date != null) { | ||
| rdfDescription.setAttributeNS("http://www.w3.org/2000/xmlns/", "xmlns:booklore", "http://booklore.org/metadata/1.0/"); | ||
| rdfDescription.appendChild(createSimpleElement(doc, "booklore:purchase_date", date.toString())); |
There was a problem hiding this comment.
In the PDF writer, when `clear` is true, `copyPurchaseDate` passes `null` to the consumer. But the consumer here only acts when `date != null`:
```java
helper.copyPurchaseDate(clear != null && clear.isPurchaseDate(), date -> {
if (date != null) {
// ...
}
});
```
This means clearing the purchase date from a PDF is a no-op: the existing `booklore:purchase_date` element won't be removed. If clearing is expected to work, you need to handle removal of the existing element when `date` is null (similar to how `replaceBookloreMetaElement` works in the EPUB writer).
Resolve conflicts keeping both upstream changes and purchase date feature additions.
- Replace FQN imports with proper imports (Instant, File) in BookUpdateService, EpubMetadataExtractor, PdfMetadataExtractor, BookUpdateServiceTest - Remove redundant @mapping for purchaseDate in BookMapper (MapStruct auto-maps) - Remove redundant user null check and bookIds null check in BookUpdateService - Add audit log.info for purchase date updates - Use findAllWithMetadataByIds for eager loading in updatePurchaseDate - Fix MetadataCopyHelper: add missing braces on else-if body - Rename misleading test: shouldFallbackToAddedOnWhenNull -> shouldSetNullWhenPurchaseDateIsNull - Fix sort.service.ts and book-table.component.ts: purchaseDate falls back to addedOn
|
Hi @acx10 / @balazs-szucs , completed your requested changes. Can you review once again? |
📝 Description
Adds end-to-end support for tracking a book’s purchase date. This includes backend persistence, APIs, permissions, and validation, plus the corresponding frontend models, UI, sorting, and stats updates.
🏷️ Type of Change
🔧 Changes
/api/v1/books/purchase-datewith authorization + future-date validation🧪 Testing
docker compose -f dev.docker-compose.yml exec backend sh -c "cd /booklore-api && ./gradlew test --tests BookUpdateServiceTest"docker compose -f dev.docker-compose.yml exec ui sh -c "cd /angular-app && npx vitest run"docker compose -f dev.docker-compose.yml exec ui sh -c "cd /angular-app && npx ng build --configuration=development"📸 Screenshots (if applicable)
✅ Pre-Submission Checklist
develop(merge conflicts resolved)./gradlew testfor backend,ng testfor frontend)🤖 AI-Assisted Contributions
TODOs, or unused scaffolding