Refactor Message model to centralize column definitions - #114
Conversation
- Move CODE_COLUMN_NAME and DEFAULT_COLUMN_NAME constants from MessageExport to Message model, leaving deprecated alias for backward compatibility - Add shared getColumns() method to Message for reuse - Update MessageImport to use the shared columns - Update MessageExport to use the shared columns, plus "found"
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds column-name constants and a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 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 `@models/MessageExport.php`:
- Around line 32-39: The export breaks when message_data is null because
array_key_exists() is invoked on a nullable value; normalize
$message->message_data at the start of the closure (e.g., $data =
$message->message_data ?: []) before using array_key_exists so the rest of the
loop (in the Message::all()->map(...) closure where $data and $columns are used)
safely treats absent JSON as an empty array.
- Line 30: The override of exportData in MessageExport.php uses strict parameter
and return type hints that conflict with Backend\Models\ExportModel::exportData;
remove the type declarations so the signature matches the untyped parent (change
"public function exportData(array $columns, ?string $sessionKey = null): array"
to an untyped "public function exportData($columns, $sessionKey = null)" while
preserving the parameter names and current implementation and return behavior),
ensuring the method no longer declares parameter or return types but still
returns the same data structure at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4233788-b201-4086-b005-db3046131881
📒 Files selected for processing (4)
controllers/Messages.phpmodels/Message.phpmodels/MessageExport.phpmodels/MessageImport.php
|
Hello Matteo, thank you for the PR. Please address CodeRabbit's flagged issues with the Import/Export model method signature. Also, please explain the use of the "found" column in the export. Thanks. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
models/MessageExport.php (1)
30-30:⚠️ Potential issue | 🔴 CriticalRestore parent-compatible
exportData()signature to avoid fatal class loading error.Line 30 is currently incompatible with
Backend\Models\ExportModel::exportData($columns, $sessionKey = null), which causes the reported fatal declaration error.Compatibility fix
-public function exportData(array $columns, ?string $sessionKey = null): array +public function exportData($columns, $sessionKey = null)#!/bin/bash # Verify parent and child method signatures are compatible. # Expected: both declarations should be untyped in parameters/return for override compatibility in this codebase. fd -t f "ExportModel.php" --exec sh -c ' echo "== {} =="; rg -n "function exportData\\s*\\(" "{}" -C2; ' \; echo "== models/MessageExport.php ==" rg -n "function exportData\\s*\\(" models/MessageExport.php -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/MessageExport.php` at line 30, The child method MessageExport::exportData currently declares typed parameters/return (array, ?string) that conflict with Backend\Models\ExportModel::exportData; update MessageExport::exportData to match the parent signature by removing parameter and return type declarations so the method signature is untyped (same parameters $columns, $sessionKey = null) and thus compatible with the parent override.
🧹 Nitpick comments (1)
models/MessageExport.php (1)
46-50: Clarify in docblock thatfoundis export-only metadata.Given reviewer feedback, consider extending the comment to explicitly state that
foundis exported for human review and intentionally excluded from import because it is recalculated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/MessageExport.php` around lines 46 - 50, Update the getColumns() docblock in the MessageExport class to state that the returned columns are the importable columns plus an export-only metadata flag named 'found' that is included for human review and intentionally excluded from import because it is recalculated; reference the method getColumns() and the Message::getColumns() call and explicitly mention the 'found' flag as export-only metadata in the comment so future readers know it is not persisted on import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@models/MessageExport.php`:
- Line 30: The child method MessageExport::exportData currently declares typed
parameters/return (array, ?string) that conflict with
Backend\Models\ExportModel::exportData; update MessageExport::exportData to
match the parent signature by removing parameter and return type declarations so
the method signature is untyped (same parameters $columns, $sessionKey = null)
and thus compatible with the parent override.
---
Nitpick comments:
In `@models/MessageExport.php`:
- Around line 46-50: Update the getColumns() docblock in the MessageExport class
to state that the returned columns are the importable columns plus an
export-only metadata flag named 'found' that is included for human review and
intentionally excluded from import because it is recalculated; reference the
method getColumns() and the Message::getColumns() call and explicitly mention
the 'found' flag as export-only metadata in the comment so future readers know
it is not persisted on import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74e92aa0-f1b0-4982-a056-5170d38e06cf
📒 Files selected for processing (1)
models/MessageExport.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
models/MessageExport.php (1)
9-19: Use/**for DocBlock comments to enable deprecation warnings.The deprecation annotations use
/*(regular comment) instead of/**(DocBlock). IDEs and documentation tools won't recognize the@deprecatedtags, so developers won't receive proper deprecation warnings when using these constants.Fix DocBlock syntax
- /* + /** * `@deprecated` since version 2.3.2, use \Winter\Translate\Models\Message::CODE_COLUMN_NAME directly * `@see` \Winter\Translate\Models\Message::CODE_COLUMN_NAME */ const CODE_COLUMN_NAME = \Winter\Translate\Models\Message::CODE_COLUMN_NAME; - /* + /** * `@deprecated` since version 2.3.2, use \Winter\Translate\Models\Message::DEFAULT_COLUMN_NAME directly * `@see` \Winter\Translate\Models\Message::DEFAULT_COLUMN_NAME */ const DEFAULT_COLUMN_NAME = \Winter\Translate\Models\Message::DEFAULT_COLUMN_NAME;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/MessageExport.php` around lines 9 - 19, The deprecation comments for the constants are regular comments, so change both comment blocks to proper DocBlock syntax (use /** ... */) above the constants CODE_COLUMN_NAME and DEFAULT_COLUMN_NAME in MessageExport so tooling/IDEs pick up the `@deprecated` and `@see` annotations; preserve the existing `@deprecated` text and the `@see` reference to \Winter\Translate\Models\Message::CODE_COLUMN_NAME and ::DEFAULT_COLUMN_NAME.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@models/MessageExport.php`:
- Around line 9-19: The deprecation comments for the constants are regular
comments, so change both comment blocks to proper DocBlock syntax (use /** ...
*/) above the constants CODE_COLUMN_NAME and DEFAULT_COLUMN_NAME in
MessageExport so tooling/IDEs pick up the `@deprecated` and `@see` annotations;
preserve the existing `@deprecated` text and the `@see` reference to
\Winter\Translate\Models\Message::CODE_COLUMN_NAME and ::DEFAULT_COLUMN_NAME.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a2701f4-2ecf-492a-aaf4-e84e292ebdd2
📒 Files selected for processing (1)
models/MessageExport.php
161283d to
8dfa28b
Compare
|
I apologize for the inaccuracies—this PR comes from work done live for a client. |
No, that's fine. What's the need for the |
|
The absence of the "found" column in the export results in the loss of a critical data point for human review. Over time, unused translations may accumulate; while they should not necessarily be removed, as they can still represent a valuable resource, the lack of the "found" field in the export makes it impossible to accurately scope and prioritize the content relevant at a given point in time. |
2 participants
The last point is particularly important to avoid losing the "found" data in the export for human review, but it is unnecessary in the import since the system recalculates it.
Summary by CodeRabbit
New Features
Bug Fixes
Chores