Page MenuHomePhorge

Projects: improve quality of destroy workflow
Needs ReviewPublic

Authored by valerio.bozzolan on Aug 12 2024, 21:21.

Details

Summary

When you permanently destroy a specific project from the command line, the following
additional good things now happen:

  • all direct milestones are removed too (since they cannot live alone, and they cannot be just "moved" to the parent project since their resulting sequence would have no sense)
    • closing T15913: direct milestones are not orphan, broken, invisible anymore, do not become junk in the database anymore, do not cause project query overhead anymore
  • all children projects emerge like cute bubbles by one level
    • closing T15918: children projects are not orphan, broken, invisible anymore
  • the parent project is eventually restored as "without sub-projects", so, joinable again
    • closing T15697: the parent project has not bugged memberships anymore

This change will only affect future usages of the destroy workflow on a project (or root-project or sub-project), that is, this command line workflow:

./bin/remove destroy PHID-OF-YOUR-PROJECT-TO-BE-DESTROYED

Limitations

This change has nothing to do with the action "Archive" on projects (btw, archiving still works and it's still very recommended).

This change does not try to improve the destruction of a specific milestone (still not recommended because milestones are in sequence and more discussion is needed to improve this corner case of the destruction of a single very specific milestone, btw it works).

This change does not try to improve the destruction of a specific workboard (btw, a workboard does not really exist, it's just a flag in the project).

Example initial situation

Project A
 [projectDepth: 0]
 [hasSubprojects: 1]
 [members: *automatic*, foo, bar]
 > Project B
  [projectDepth: 1]
  [hasSubprojects: 1]
  [members: *automatic*, foo, bar]
  > Project C
   [hasSubprojects: 0]
   [projectDepth: 2]
   [members: foo, bar]
   > Milestone C.1
    [projectDepth: 3]

Situation after destroying only project A:

Project B
 [projectDepth: 0]
 [hasSubprojects: 1]
 [members: *automatic*, foo, bar]
 > Project C
  [projectDepth: 1]
  [hasSubprojects: 0]
  [members: foo, bar]
  > Milestone C.1
   [projectDepth: 2]

Situation after destroying only project B:

Project A
 [projectDepth: 0]
 [hasSubprojects: 1]
 [members: *automatic*, foo, bar]
 > Project C
  [projectDepth: 1]
  [hasSubprojects: 0]
  [members: foo, bar]
  > Milestone C.1
   [projectDepth: 2]

Situation after destroying only project C:

Project A
 [projectDepth: 0]
 [hasSubprojects: 1]
 [members: *automatic*, foo, bar]
 > Project B
  [projectDepth: 1]
  [hasSubprojects: 0]
  [members: foo, bar]

Situation after destroying only milestone C.1:

Project A
 [projectDepth: 0]
 [hasSubprojects: 1]
 [members: *automatic*, foo, bar]
 > Project B
  [projectDepth: 1]
  [hasSubprojects: 1]
  [members: *automatic*, foo, bar]
  > Project C
   [hasSubprojects: 0]
   [projectDepth: 2]
   [members: foo, bar]

Closes T15697
Closes T15913
Closes T15918
Closes T16043

Test Plan

Run multiple times the destroy workflow under every imaginable case,
to violently destroy a projects tree, leaf by leaf. We suggest to run it
with the trace option. Example:

./bin/remove destroy --trace PHID-PROJ-s0met1ng

To run the above workflow multiple times, you may appreciate a dump:

./bin/storage dump | gzip > dump.sql.gz

So you can easily restore it whenever you want:

zcat dump.sql.gz | ./bin/storage shell
  • Delete simple milestone
    • create a new simple project "A" with milestone "M" and delete "M"
    • deletion still works on milestone "M"
  • Delete simple project
    • create a new simple project "A" with a workboard and destroy "A":
    • deletion still works on project "A"
    • workboard is still not accessible
    • if there were tasks, they are still there but without tag "A"
  • Delete project with milestone (T15913)
    • create a new project "A" with a milestone "B" and delete "A"
    • deletion still works on project "A"
    • milestone "B" is finally nuked from the database and you do not see it anymore in the phabricator_project table
  • Delete a project with sub-projects (T15918)
    • create a new project "A" with a sub-project called "B" and delete "A"
    • deletion still works on project "A"
    • project "B" becomes a root-project and it has depth "0" again and it's usable, instead of having it borked with "You Shall Not Pass"
  • Delete a sub-project (T15697)
    • create a new project "A" with a sub-project called "B" and delete "B"
    • deletion still works on project "B"
    • project "A" is usable again like "B" never existed, so you can change memberships and all the things, instead of having it borked

If you don't have enough time to test this, just enjoy the new unit tests covering the destroy process (T16043):

arc unit src/applications/project/storage/PhabricatorProject.php

In particular, in the unit test results, check the new green light about this method:

PhabricatorProjectCoreTestCase::testProjectDestroy

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25772_1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1743
Build 1743: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
src/docs/user/userguide/projects.diviner
362
369
375
376
390
393โ€“394

OK my friend, I've worked a lot to make it shorter... but still more dense of useful info, reducing jokes at minimum.

Really much more info in less text.

Sorry for extra major reading lol

Aldo I may need extra help from @waldyrious asdlol (on the Documentation I mean)

clarify how to run the command (last change promise)

Hi @mainframe98 - I still see your block. Do you like the new documentation now? ๐Ÿ‘

valerio.bozzolan marked 14 inline comments as done.

implement documentation tips - thanks - also re-wrap some lines to be under 80 chars

The docs are surprisingly less jokey than I feared. Left a few minor change suggestions, as well as in the code comments.

src/applications/project/storage/PhabricatorProject.php
753

Small suggestion to make the sentence more readable (IMHO)

763

In this case "gaps" fits a bit better than "holes".

766
802

Not sure if this is accurate, but something along these lines is easier to understand IMHO.

src/docs/user/userguide/projects.diviner
346
351

Just a suggestion :P

354
362โ€“363

It's quite unclear to me what this is supposed to mean.

364
366
371
379

Shouldn't "Policies In Depth" be a link?

380

Also, wouldn't "a specific team" or "a specific group" be sufficient? "team group" seems redundant.

383โ€“387
394โ€“395
404
408
410
413
418
421โ€“423
424โ€“425
426

Not sure if "to" is appropriate here. Perhaps "as another project" might be closer to the original intent?

428
431
432โ€“434
435โ€“436
439
440

I think the repetition reinforces the sequential stepping up we're evoking here.

454
455
461

add destroy unit test for A > B > C and milestone (I love computers)

valerio.bozzolan marked 28 inline comments as done.

fix damn 81 lines lint

valerio.bozzolan marked 4 inline comments as done.

removed nonsense comment (last change.zip)

valerio.bozzolan edited the test plan for this revision. (Show Details)

Can we also destroy the root project A? Sure! Added in the unit test.

Also, added more inline documentation.

Also, added more test utilities (e.g. we can easily test if a join crashes in 1 line).

Reword inline comment to be about our hero, Mario.

Added a "Limitations" section to clarify what this patch is not about, and clarifying why direct Milestones must be removed (so you don't need to read all the subtasks)

I'm happy because when a coworker creates a new sub-project "Nonsense Sub", now you can just destroy it (the project, not the coworker lol) and everything is rollbacked as before successfully.

No anymore side-effects like T15918, T15913, T15697.