fix: normalize translator/illustrator/curator names (#93)#94
Open
fabiodalez-dev wants to merge 2 commits intomainfrom
Open
fix: normalize translator/illustrator/curator names (#93)#94fabiodalez-dev wants to merge 2 commits intomainfrom
fabiodalez-dev wants to merge 2 commits intomainfrom
Conversation
Apply AuthorNormalizer::normalize() to traduttore, illustratore, and curatore fields in BookRepository::createBasic(), updateBasic(), and updateOptionals(). This ensures the same name standardization that authors receive (title case, "Surname, Name" → "Name Surname", particle handling for de/von/di/etc.) is also applied to these roles. Also normalize scraped translator/illustrator values in the JavaScript form using the existing normalizeAuthorName() function. Fixes #93
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughApply consistent normalization of translator, illustrator, and curator names in the form (including ISBN/scrape paths) and repository create/update methods; also add Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Browser
participant FormJS as "BookForm JS"
participant Normalizer as "AuthorNormalizer"
participant Repo as "BookRepository"
participant DB as "Database"
rect rgba(200,200,255,0.5)
User->>FormJS: trigger ISBN import / fill form
FormJS->>Normalizer: normalizeAuthorName(translator)
FormJS->>Normalizer: normalizeAuthorName(illustrator)
FormJS->>FormJS: populate inputs and scraped_* with normalized names
end
rect rgba(200,255,200,0.5)
FormJS->>Repo: submit form data (normalized fields)
Repo->>Normalizer: normalize(traduttore/illustratore/curatore) when present
Repo->>DB: UPDATE/INSERT normalized fields (WHERE ... AND deleted_at IS NULL)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/BookRepository.php`:
- Around line 998-1000: The updateOptionals() logic updates the libri table by
id but omits soft-delete filtering, allowing updates to soft-deleted rows;
update the database queries in updateOptionals() (and the similar update blocks
around the same code paths) so they include a condition limiting affected rows
to non-deleted records (deleted_at IS NULL or the framework's soft-delete
predicate) when targeting the libri table by id, ensuring only non-deleted libri
are modified.
- Around line 369-370: In createBasic() and updateBasic(), prevent TypeError by
only calling \App\Support\AuthorNormalizer::normalize when the incoming
traduttore value is a string and not empty; for example, where $val =
$data['traduttore'] ?? null replace the unconditional normalize call with a
guard that checks is_string($val) && $val !== '' and only then calls
AuthorNormalizer::normalize($val), otherwise pass the original $val (null or the
non-string) through — mirror the defensive pattern used in updateOptionals()
(which casts/guards) so non-string array payloads don't trigger normalize(string
$name).
In `@app/Views/libri/partials/book_form.php`:
- Around line 3731-3758: The translator and illustrator handlers reference
normalizeAuthorName which is currently declared inside the authors conditional
(authorsChoice ...) and thus can be undefined; move or hoist the
normalizeAuthorName function out of that conditional into a module/global scope
so it is available to the translator and illustrator blocks (and any other
callers), then update references in the translator (input[name="traduttore"],
scraped_translator) and illustrator (input[name="illustratore"],
scraped_illustrator) try-catch blocks to use the now-top-level
normalizeAuthorName; ensure no duplicate definitions remain inside the authors
conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fe152298-7d1b-42bd-a9a3-71e19a4c8635
📒 Files selected for processing (2)
app/Models/BookRepository.phpapp/Views/libri/partials/book_form.php
There was a problem hiding this comment.
♻️ Duplicate comments (3)
app/Views/libri/partials/book_form.php (1)
3731-3758:⚠️ Potential issue | 🟠 Major
normalizeAuthorNamecan be undefined in translator/illustrator handlers.At Line [3734] and Line [3750],
normalizeAuthorName(...)is used outside the block where it is declared (Line [3404]). When that authors block doesn’t run, these calls fail and normalization is skipped.🐛 Proposed fix
+ const normalizeRoleName = (name) => { + if (typeof normalizeAuthorName === 'function') { + return normalizeAuthorName(name); + } + const value = (name || '').trim(); + if (!value.includes(',')) return value; + const [surname, firstName] = value.split(',', 2).map((p) => p.trim()); + return surname && firstName ? `${firstName} ${surname}` : value; + }; + // Handle translator (traduttore) — normalize same as authors try { if (data.translator) { - const normalized = normalizeAuthorName(data.translator); + const normalized = normalizeRoleName(data.translator); @@ // Handle illustrator (illustratore) — normalize same as authors try { if (data.illustrator) { - const normalized = normalizeAuthorName(data.illustrator); + const normalized = normalizeRoleName(data.illustrator);#!/bin/bash # Verify declaration scope vs usage for normalizeAuthorName in ISBN import flow rg -n "const normalizeAuthorName|normalizeAuthorName\\(data\\.(translator|illustrator)\\)" app/Views/libri/partials/book_form.php sed -n '3395,3765p' app/Views/libri/partials/book_form.php | nl -ba🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/partials/book_form.php` around lines 3731 - 3758, The translator and illustrator handlers call normalizeAuthorName (used with data.translator and data.illustrator and applied to inputs named "traduttore"/"illustratore" and IDs "scraped_translator"/"scraped_illustrator") but that function is declared only inside the authors block, so it may be undefined; fix by hoisting or moving the normalizeAuthorName declaration to a shared scope above these handlers (or, alternatively, guard the calls with typeof normalizeAuthorName === 'function' and fall back to using the raw value) so both the translator and illustrator handlers can always call it safely.app/Models/BookRepository.php (2)
983-1043:⚠️ Potential issue | 🟠 Major
updateOptionals()update query still omits soft-delete filter.At Line [1042], the
UPDATE libri ... WHERE id=?statement can modify soft-deleted rows. This method should apply the same non-deleted predicate used elsewhere.✅ Proposed fix
- $sql = 'UPDATE libri SET ' . implode(', ', $set) . ', updated_at=NOW() WHERE id=?'; + $sql = 'UPDATE libri SET ' . implode(', ', $set) . ', updated_at=NOW() WHERE id=? AND deleted_at IS NULL';As per coding guidelines, "Every query on
libritable MUST include soft-delete condition:AND deleted_at IS NULL.#!/bin/bash # Verify soft-delete predicate presence in updateOptionals query rg -n "function updateOptionals|UPDATE libri SET|deleted_at IS NULL" app/Models/BookRepository.php -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 983 - 1043, The UPDATE in updateOptionals builds $sql = 'UPDATE libri SET ... WHERE id=?' which can affect soft-deleted rows; change the WHERE to include the soft-delete predicate (e.g. 'WHERE id=? AND deleted_at IS NULL') when building $sql in updateOptionals so it matches other queries on the libri table, and ensure the parameter binding/order for the id (types and $vals) remains correct after this change.
369-378:⚠️ Potential issue | 🟠 MajorGuard role-name normalization to strings only.
At Line [370], Line [374], and Line [378], non-string payloads can hit
AuthorNormalizer::normalize(...)under strict types. Also, Line [999], Line [1022], and Line [1025] currently coerce non-strings to text (e.g. arrays →"Array"), which can store invalid values.🛡️ Proposed fix
if ($this->hasColumn('traduttore')) { $val = $data['traduttore'] ?? null; - $addField('traduttore', 's', $val !== null && $val !== '' ? \App\Support\AuthorNormalizer::normalize($val) : $val); + $val = is_string($val) ? trim($val) : null; + $addField('traduttore', 's', $val !== null && $val !== '' ? \App\Support\AuthorNormalizer::normalize($val) : null); } @@ if ($this->hasColumn('illustratore')) { $val = $data['illustratore'] ?? null; - $addField('illustratore', 's', $val !== null && $val !== '' ? \App\Support\AuthorNormalizer::normalize($val) : $val); + $val = is_string($val) ? trim($val) : null; + $addField('illustratore', 's', $val !== null && $val !== '' ? \App\Support\AuthorNormalizer::normalize($val) : null); } @@ if ($this->hasColumn('curatore')) { $val = $data['curatore'] ?? null; - $addField('curatore', 's', $val !== null && $val !== '' ? \App\Support\AuthorNormalizer::normalize($val) : $val); + $val = is_string($val) ? trim($val) : null; + $addField('curatore', 's', $val !== null && $val !== '' ? \App\Support\AuthorNormalizer::normalize($val) : null); }#!/bin/bash # Verify strict typing + normalize contract + current callsites rg -n "declare\\(strict_types=1\\)|AuthorNormalizer::normalize\\(|\\['(traduttore|illustratore|curatore)'\\]" app/Models/BookRepository.php rg -n "function normalize\\(" app/Support/AuthorNormalizer.phpAlso applies to: 705-714, 998-1000, 1021-1025
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Models/BookRepository.php` around lines 369 - 378, The role-name normalization calls (for keys 'traduttore', 'illustratore', 'curatore' inside BookRepository methods that use $addField and hasColumn) must only pass string values into AuthorNormalizer::normalize to avoid strict_types failures and accidental coercion like "Array"; update each site (the blocks referencing AuthorNormalizer::normalize and the earlier similar blocks around 705-714, 998-1000, 1021-1025) to guard with an is_string($val) check (and non-empty) before calling AuthorNormalizer::normalize($val), otherwise leave $val as-is (null or original non-string) so non-string payloads are not coerced or normalized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/Models/BookRepository.php`:
- Around line 983-1043: The UPDATE in updateOptionals builds $sql = 'UPDATE
libri SET ... WHERE id=?' which can affect soft-deleted rows; change the WHERE
to include the soft-delete predicate (e.g. 'WHERE id=? AND deleted_at IS NULL')
when building $sql in updateOptionals so it matches other queries on the libri
table, and ensure the parameter binding/order for the id (types and $vals)
remains correct after this change.
- Around line 369-378: The role-name normalization calls (for keys 'traduttore',
'illustratore', 'curatore' inside BookRepository methods that use $addField and
hasColumn) must only pass string values into AuthorNormalizer::normalize to
avoid strict_types failures and accidental coercion like "Array"; update each
site (the blocks referencing AuthorNormalizer::normalize and the earlier similar
blocks around 705-714, 998-1000, 1021-1025) to guard with an is_string($val)
check (and non-empty) before calling AuthorNormalizer::normalize($val),
otherwise leave $val as-is (null or original non-string) so non-string payloads
are not coerced or normalized.
In `@app/Views/libri/partials/book_form.php`:
- Around line 3731-3758: The translator and illustrator handlers call
normalizeAuthorName (used with data.translator and data.illustrator and applied
to inputs named "traduttore"/"illustratore" and IDs
"scraped_translator"/"scraped_illustrator") but that function is declared only
inside the authors block, so it may be undefined; fix by hoisting or moving the
normalizeAuthorName declaration to a shared scope above these handlers (or,
alternatively, guard the calls with typeof normalizeAuthorName === 'function'
and fall back to using the raw value) so both the translator and illustrator
handlers can always call it safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4002068-28e7-4b88-8bf1-d11850cf284b
📒 Files selected for processing (2)
app/Models/BookRepository.phpapp/Views/libri/partials/book_form.php
- Guard AuthorNormalizer::normalize() with is_string() + trim() in createBasic() and updateBasic() to prevent TypeError from non-string payloads under strict_types=1 - Add AND deleted_at IS NULL to updateOptionals() UPDATE query per project soft-delete rule - Hoist normalizeAuthorName() out of the authors conditional block so translator/illustrator handlers can always access it
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Models/BookRepository.php`:
- Around line 1004-1006: In updateOptionals(), avoid blindly string-casting
arbitrary payloads before calling \App\Support\AuthorNormalizer::normalize for
the roles ['traduttore','illustratore','curatore']; add the same guard used in
createBasic()/updateBasic() so you only normalize real non-empty string input
(e.g. check that $data[$c] is a string/scalar and that trim((string)$data[$c])
!== '' or equivalent) before assigning $cols[$c] =
AuthorNormalizer::normalize(...), otherwise skip setting the column; apply the
same guard to the other similar block at the region corresponding to lines
1027-1031.
In `@app/Views/libri/partials/book_form.php`:
- Around line 3398-3412: The normalizeAuthorName function currently only
reorders "Surname, Name" but doesn't apply the repository's casing/particle
rules; update normalizeAuthorName to mirror
App\Support\AuthorNormalizer::normalize by normalizing all-caps input to Title
Case, preserving and lowercasing name particles (e.g., "von", "de", "da") as
AuthorNormalizer does, and applying the same handling for initials and
multi-part surnames so imported values match saved repository state; locate the
normalizeAuthorName arrow function in the form script and either call a new
endpoint that runs AuthorNormalizer::normalize or reimplement the same
normalization rules (matching particle list and casing rules) inside
normalizeAuthorName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1e25ea5-dcc4-43be-8860-939fab5885af
📒 Files selected for processing (2)
app/Models/BookRepository.phpapp/Views/libri/partials/book_form.php
Comment on lines
+1004
to
1006
| } elseif (in_array($c, ['traduttore', 'illustratore', 'curatore'], true)) { | ||
| $cols[$c] = \App\Support\AuthorNormalizer::normalize((string) $data[$c]); | ||
| } else { |
There was a problem hiding this comment.
Apply the same guard in updateOptionals().
This path still string-casts arbitrary payloads before normalizing. That means whitespace-only values can end up as '', and malformed array payloads can still be persisted as "Array", even though createBasic() / updateBasic() now reject them.
🛠️ Proposed fix
- } elseif (in_array($c, ['traduttore', 'illustratore', 'curatore'], true)) {
- $cols[$c] = \App\Support\AuthorNormalizer::normalize((string) $data[$c]);
+ } elseif (in_array($c, ['traduttore', 'illustratore', 'curatore'], true)) {
+ $value = is_string($data[$c]) ? trim($data[$c]) : null;
+ if ($value !== null && $value !== '') {
+ $cols[$c] = \App\Support\AuthorNormalizer::normalize($value);
+ }
} else {
$cols[$c] = $data[$c];
}
@@
- if ($this->hasColumn('traduttore') && !isset($cols['traduttore']) && !empty($data['scraped_translator'])) {
- $cols['traduttore'] = \App\Support\AuthorNormalizer::normalize((string) $data['scraped_translator']);
+ if ($this->hasColumn('traduttore') && !isset($cols['traduttore'])) {
+ $value = is_string($data['scraped_translator'] ?? null) ? trim($data['scraped_translator']) : '';
+ if ($value !== '') {
+ $cols['traduttore'] = \App\Support\AuthorNormalizer::normalize($value);
+ }
}
- if ($this->hasColumn('illustratore') && !isset($cols['illustratore']) && !empty($data['scraped_illustrator'])) {
- $cols['illustratore'] = \App\Support\AuthorNormalizer::normalize((string) $data['scraped_illustrator']);
+ if ($this->hasColumn('illustratore') && !isset($cols['illustratore'])) {
+ $value = is_string($data['scraped_illustrator'] ?? null) ? trim($data['scraped_illustrator']) : '';
+ if ($value !== '') {
+ $cols['illustratore'] = \App\Support\AuthorNormalizer::normalize($value);
+ }
}Also applies to: 1027-1031
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Models/BookRepository.php` around lines 1004 - 1006, In
updateOptionals(), avoid blindly string-casting arbitrary payloads before
calling \App\Support\AuthorNormalizer::normalize for the roles
['traduttore','illustratore','curatore']; add the same guard used in
createBasic()/updateBasic() so you only normalize real non-empty string input
(e.g. check that $data[$c] is a string/scalar and that trim((string)$data[$c])
!== '' or equivalent) before assigning $cols[$c] =
AuthorNormalizer::normalize(...), otherwise skip setting the column; apply the
same guard to the other similar block at the region corresponding to lines
1027-1031.
Comment on lines
+3398
to
+3412
| // Normalize person names: "Surname, Name" → "Name Surname" (shared by authors, translator, illustrator) | ||
| const normalizeAuthorName = (name) => { | ||
| name = (name || '').trim(); | ||
| if (name.includes(',')) { | ||
| const parts = name.split(',', 2); | ||
| if (parts.length === 2) { | ||
| const surname = parts[0].trim(); | ||
| const firstName = parts[1].trim(); | ||
| if (surname && firstName) { | ||
| return firstName + ' ' + surname; | ||
| } | ||
| } | ||
| } | ||
| return name; | ||
| }; |
There was a problem hiding this comment.
Mirror AuthorNormalizer in the import helper.
normalizeAuthorName() only reorders Surname, Name. It still leaves all-caps input and particle casing untouched, so importing ROSSI, MARIO / VON HAGEN, VICTOR will still populate the form with MARIO ROSSI / VICTOR VON HAGEN. If you want the imported form state to match what the repository now saves, this helper needs the same normalization rules as App\Support\AuthorNormalizer::normalize().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/libri/partials/book_form.php` around lines 3398 - 3412, The
normalizeAuthorName function currently only reorders "Surname, Name" but doesn't
apply the repository's casing/particle rules; update normalizeAuthorName to
mirror App\Support\AuthorNormalizer::normalize by normalizing all-caps input to
Title Case, preserving and lowercasing name particles (e.g., "von", "de", "da")
as AuthorNormalizer does, and applying the same handling for initials and
multi-part surnames so imported values match saved repository state; locate the
normalizeAuthorName arrow function in the form script and either call a new
endpoint that runs AuthorNormalizer::normalize or reimplement the same
normalization rules (matching particle list and casing rules) inside
normalizeAuthorName.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AuthorNormalizer::normalize()totraduttore,illustratore, andcuratorefields inBookRepository(create, update, and scraping paths)normalizeAuthorName()to scraped translator/illustrator values in the book formPreviously, only author names were standardized (title case, "Surname, Name" → "Name Surname", particle handling for de/von/di/etc.). Translator, illustrator, and curator fields were saved as raw text, leading to inconsistent formatting.
Test plan
Fixes #93
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
User Interface