fix: set createdAt timestamp for local attachments#11966
Conversation
kesselb
force-pushed
the
bug/5901/set-created-at-on-insert
branch
from
5ac547c to
4f09d8c
Compare
kesselb
force-pushed
the
bug/5901/set-created-at-on-insert
branch
2 times, most recently
from
2f91323 to
24fd083
Compare
There was a problem hiding this comment.
Pull request overview
Updates local attachment creation to always set a createdAt timestamp via ITimeFactory, and aligns tests and wiring to the new dependency.
Changes:
- Inject
ITimeFactoryintoAttachmentServiceand setcreatedAton new local attachments. - Update unit/integration tests to provide a time factory and assert
createdAt. - Add a DB migration related to
mail_attachments.created_atand bump app/package versions.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Service/Attachment/AttachmentService.php | Injects ITimeFactory and sets createdAt during attachment creation |
| tests/Unit/Service/Attachment/AttachmentServiceTest.php | Mocks ITimeFactory and asserts createdAt in expected attachments |
| tests/Integration/Service/OutboxServiceIntegrationTest.php | Passes ITimeFactory to service construction in integration setup |
| tests/Integration/Service/DraftServiceIntegrationTest.php | Passes ITimeFactory to service construction in integration setup |
| lib/Migration/Version5008Date20260320000001.php | Adds migration adjusting mail_attachments.created_at default |
| package.json | Updates package version to 5.8.0-alpha.1 |
| appinfo/info.xml | Updates app version to 5.8.0-alpha.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @param Closure(): ISchemaWrapper $schemaClosure | ||
| */ | ||
| #[\Override] |
There was a problem hiding this comment.
#[\\Override] is a PHP 8.3+ built-in attribute; if this app supports PHP 8.1/8.2 (common for Nextcloud apps), loading this file will fatal with Attribute class \"Override\" not found. To keep compatibility, remove the attribute (or use a project-provided override attribute if the codebase has one) and rely on the method signature/interface contract.
| #[\Override] |
| { | ||
| "name": "nextcloud-mail", | ||
| "version": "5.7.0-rc.1", | ||
| "version": "5.8.0-alpha.1", |
There was a problem hiding this comment.
The PR description focuses on setting createdAt and updating tests/wiring, but this change also bumps the app/package version (and appinfo/info.xml does the same). Either update the PR description to explicitly call out the version bump (and why it’s needed for this change/migration), or avoid changing versions in this PR to keep the scope aligned.
| /** @var ITimeFactory|MockObject */ | ||
| private $timeFactory; |
There was a problem hiding this comment.
This mock property is untyped, which weakens static analysis/autocomplete and can hide type mistakes. If the project’s PHP version allows it, prefer an intersection type on the property (e.g., private ITimeFactory&MockObject $timeFactory;) and drop the phpdoc, or at least add a concrete property type plus phpdoc for the mock interface.
| /** @var ITimeFactory|MockObject */ | |
| private $timeFactory; | |
| private ITimeFactory&MockObject $timeFactory; |
kesselb
force-pushed
the
bug/5901/set-created-at-on-insert
branch
from
ba1d2f9 to
3228e8d
Compare
- Inject ITimeFactory into AttachmentService constructor - Set createdAt using timeFactory.getTime() when creating attachments - Update unit tests to mock ITimeFactory and expect createdAt value - Update integration tests to pass ITimeFactory dependency Signed-off-by: dhairyasquad73 <dhairya.jangir.s73@kalvium.community> Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
kesselb
force-pushed
the
bug/5901/set-created-at-on-insert
branch
from
3228e8d to
29f84a6
Compare
|
Backport or is it too risky? |
|
Not really necessary, I'd say. |
Local copy of #11943 with squashed commits.