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 Passed
Unit
No Test Coverage
Build Status
Buildable 1123
Build 1123: 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
48

pht()

160–168

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

236

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
160–168

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
160–168

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.