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:

  • the direct orhpan milestones are removed too (instead of leaving them orphan, broken, invisible forever)
  • all children projects emerge like cute bubbles by one level (instead of having them orphan, broken, invisible forever)
  • the parent project is eventually restored as "without sub-projects" (instead of having a broken root-project with bugged memberships)

This change has nothing to do with archived projects.

This change will only affect future usages of the destroy workflow, that is:

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

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 1920
Build 1920: 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
387–389

The risks are explained after "What happens when you permanently destroy" 👍 (is this the answer?)

391

Here I would just like to clarify that a milestone is really a project. I put "or a milestone" as solution

valerio.bozzolan marked 4 inline comments as done.

add missing comma

last cosmetic change on a title with extra "==="

clarify that an archived tag is not removed from their tasks

Dear friend @mainframe98 - I suggest to click the button Hide "Done" inlines in the top-right corner to easily do another reading

This is much better. A few spelling errors remain, plus two suggestions to make sentences read a little less awkward.

src/docs/user/userguide/projects.diviner
354
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)