Page MenuHomePhorge

Initial work
ClosedPublic

Authored by tsc on Mar 23 2024, 14:00.

Details

Summary

This revision is not like another 😉

I want to collect your feedback If I am doing it right. So please not only consider this change, but the whole repository under R13. I'm implementing my first extension for Phorge and am very new to the entire code base. I'm trying to please the linting gods, but sure there is stuff that I'm missing, and I am doing wrong.

Test Plan

Start devcontainer supplied by this project, login as admin, create a task and clone it.

Diff Detail

Repository
R13 DeepClone
Branch
arcpatch-D25558
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/deepclone/controller/DeepcloneController.php:3PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:5PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:12PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:15PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:22PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:34PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:47PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:49PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:52PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:74PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:79PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:86PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:118PHL1Unknown Symbol
Errorsrc/deepclone/controller/DeepcloneController.php:125PHL1Unknown Symbol
Warningsrc/deepclone/controller/DeepcloneController.php:57XHP37Call Formatting
Warningsrc/deepclone/controller/DeepcloneController.php:65XHP37Call Formatting
Warningsrc/deepclone/controller/DeepcloneController.php:112TXT3Line Too Long
Warningsrc/deepclone/controller/DeepcloneController.php:118XHP9Naming Conventions
Warningsrc/deepclone/controller/DeepcloneController.php:120XHP9Naming Conventions
Warningsrc/deepclone/controller/DeepcloneController.php:121XHP9Naming Conventions
Warningsrc/deepclone/controller/DeepcloneController.php:122XHP9Naming Conventions
Warningsrc/deepclone/controller/DeepcloneController.php:123XHP9Naming Conventions
Warningsrc/deepclone/controller/DeepcloneController.php:124XHP9Naming Conventions
Warningsrc/deepclone/controller/DeepcloneController.php:125XHP9Naming Conventions
Warningsrc/deepclone/controller/DeepcloneController.php:126XHP9Naming Conventions
Unit
No Test Coverage
Build Status
Buildable 1117
Build 1117: arc lint + arc unit

Event Timeline

tsc requested review of this revision.Mar 23 2024, 14:00

Continued work, now using proper transactions and beeing able to copy comments

Implement the actual deep clone

IIRC, The save() command won't generate any Feed item, so the unified feed entry would be either "xxx triaged this task" or "xxx added a comment". Also, for each comment added, a new Feed item will be generated, which is probably not what you'd like.

I think you can get around this by adding a Create xaction to the list, which will instruct the Feed system to hide all xactions and just show "xxx created this" - see T15754. You could even create a custom xaction for "xxx cloned this from that", but that's probably a bit more involved.

I'm curious about the use-case for copying comments as well - I'm guessing that there's some extra data that's added in the comments?

Overall, this looks like it's in the right direction - I don't see anything "wrong" with this implementation.

src/deepclone/controller/DeepcloneController.php
66

pht()

182–190

All of these will happen w/o a transaction - I'm guessing you're ok with that?

258

returning 2 different things looks strange...

Remove comment cloning

Add a transaction for cloning the task

Address suggested changes by @avivey

Fix local setup

tsc marked 2 inline comments as done.EditedApr 3 2024, 09:10

I'm curious about the use-case for copying comments as well - I'm guessing that there's some extra data that's added in the comments?

I added that because I was inspired by that on another platform. But I removed it because I think it will not generate enough value.

IIRC, The save() command won't generate any Feed item, so the unified feed entry would be either "xxx triaged this task" or "xxx added a comment". Also, for each comment added, a new Feed item will be generated, which is probably not what you'd like.
I think you can get around this by adding a Create xaction to the list, which will instruct the Feed system to hide all xactions and just show "xxx created this" - see T15754. You could even create a custom xaction for "xxx cloned this from that", but that's probably a bit more involved.

I added two transactions, one for the source and one for the target.

This is what it looks like on the target:

{F2114171}

And this on the source:

{F2114184}

Overall, this looks like it's in the right direction - I don't see anything "wrong" with this implementation.

Thanks for your feedback!

I think I'm pleased with it now and would it called finished. I hope I did not miss anything.

src/deepclone/controller/DeepcloneController.php
182–190

I think I'm quite fine with that. Are there any repercussions on that?

(The images aren't public/attached, so I can't see them)

src/deepclone/controller/DeepcloneController.php
182–190

Probably only the Feed entry, and maybe the new task won't be added to the Search index - but both of this issues would be covered by the PhabricatorCoreCreateTransaction in any case.

There may be a strange interaction with Herald, but I think Herald doesn't look at Transactions anyway. Or if it does, then it will be handled in the next transactions anyway (Herald doesn't bother looking at transactions, it checks out the "current" state of the object - at least for most cases).

tsc marked 2 inline comments as done.Apr 3 2024, 12:12

(The images aren't public/attached, so I can't see them)

Fixed that.

From my standpoint, this would be finished. How does this work that this change can land on the main branch?

You should be able to just arc land this now...

This revision is now accepted and ready to land.Apr 4 2024, 08:00
This revision was automatically updated to reflect the committed changes.