Maniphest - use subscribers framework properly
ClosedPublic
Actions

Authored by btrahan on Dec 10 2014, 11:38 PM.

Details

Reviewers

epriestley

Maniphest Tasks

T5604: Properly integrate Maniphest with Subscribers

Commits

Restricted Diffusion Commit
rP7d9687057056: Maniphest - use subscribers framework properly

Summary

Fixes T5604. This should fix some random bugs, lets us move forward more easily, and all that good stuff about killing code debt.

Test Plan

Conduit method maniphest.createtask
- verified creating user subscribed
- verified subscription transaction
Conduit method maniphest.update
- verified subscribers set as specified to ccPHIDs parameter
- verified subscription transaction
Herald
- verified herald rule to add subscriber worked
- verified no subscribers removed accidentally
edit controller
- test create and verify author gets added IFF they put themselves in subscribers control box
- test update gets set to exactly what user enters
lipsum generator'd tasks work
bulk add subscribers works
bulk remove subscriber works
detail controller
- added myself by leaving a comment
- added another user via explicit action
- added another user via implicit mention
task merge via search attach controller
mail reply handler
- add subscriber via ./bin/mail receive-test
- unsubscribe via ./bin/mail receive-test

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

btrahan updated this revision to Diff 26331.Dec 10 2014, 11:38 PM

btrahan added a task: T5604: Properly integrate Maniphest with Subscribers.

btrahan retitled this revision from to Maniphest - use subscribers framework properly.

btrahan updated this object.

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

btrahan added a reviewer: epriestley.

Herald added subscribers: epriestley, Korvin. · View Herald TranscriptDec 10 2014, 11:38 PM

So something is wrong with my sql change I guess? I can't get the debug information to find out what though....

15:38:59 ~/Dropbox/code/phalanx (T5604)
~> arc unit --trace
libphutil loaded from '/Users/btrahan/Dropbox/code/libphutil/src'.
arcanist loaded from '/Users/btrahan/Dropbox/code/arcanist/src'.
Config: Reading user configuration file "/Users/btrahan/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/Users/btrahan/Dropbox/code/phalanx/.arcconfig".
Working Copy: Path "/Users/btrahan/Dropbox/code/phalanx" is part of `git` working copy "/Users/btrahan/Dropbox/code/phalanx".
Working Copy: Project root is at "/Users/btrahan/Dropbox/code/phalanx".
Config: Did not find local configuration at "/Users/btrahan/Dropbox/code/phalanx/.git/arc/config".
Loading phutil library from '/Users/btrahan/Dropbox/code/phalanx/src'...
>>> [0] <exec> $ git rev-parse --verify HEAD^
<<< [0] <exec> 7,483 us
>>> [1] <exec> $ git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}'
<<< [1] <exec> 5,089 us
>>> [2] <exec> $ git rev-parse --git-dir
<<< [2] <exec> 4,921 us
>>> [3] <exec> $ git cat-file -t 'origin/master'
<<< [3] <exec> 6,291 us
>>> [4] <exec> $ git merge-base 'origin/master' HEAD
<<< [4] <exec> 6,674 us
>>> [5] <exec> $ git diff --no-ext-diff --no-textconv --raw '9b865f18e845c81d999b5aa2b9c25a4a02b838f1' --
<<< [5] <exec> 31,270 us
>>> [6] <exec> $ git diff --no-ext-diff --no-textconv --raw 'HEAD' --
>>> [7] <exec> $ git ls-files --others --exclude-standard
<<< [7] <exec> 26,609 us
<<< [6] <exec> 33,015 us
>>> [8] <exec> $ git diff-files --name-only
<<< [8] <exec> 12,288 us
>>> [9] <connect> phabricator_config
<<< [9] <connect> 424 us
>>> [10] <query> SELECT * FROM `config_entry` WHERE namespace = 'default' AND isDeleted = 0
<<< [10] <query> 331 us
>>> [11] <exec> $ php '/Users/btrahan/Dropbox/code/phalanx/scripts/sql/manage_storage.php' upgrade --force --namespace 'phabricator_unittest_d6guujbzyuk7ew4hwkmtunu2'
<<< [11] <exec> 4,002,952 us

[2014-12-10 15:39:12] EXCEPTION: (CommandException) Command failed with error #2!
COMMAND
php '/Users/btrahan/Dropbox/code/phalanx/scripts/sql/manage_storage.php' upgrade --force --namespace 'phabricator_unittest_d6guujbzyuk7ew4hwkmtunu2'

STDOUT
Loading quickstart template...
Applying patch 'phabricator:20141106.uniqdrafts.php'...
Applying patch 'phabricator:20141107.phriction.policy.1.sql'...
Applying patch 'phabricator:20141107.phriction.policy.2.php'...
Populating Phriction policies.
Done.
Applying patch 'phabricator:20141107.phriction.popkeys.php'...
Populating Phriction mailkeys.
Done.
Applying patch 'phabricator:20141107.ssh.1.colname.sql'...
Applying patch 'phabricator:20141107.ssh.2.keyhash.sql'...
Applying patch 'phabricator:20141107.ssh.3.keyindex.sql'...
Applying patch 'phabricator:20141107.ssh.4.keymig.php'...
Updating SSH public key indexes...
Done.
Applying patch 'phabricator:20141107.ssh.5.indexnull.sql'...
Applying patch 'phabricator:20141107.ssh.6.indexkey.sql'...
Applying patch 'phabricator:20141107.ssh.7.colnull.sql'...
Applying patch 'phabricator:20141113.auditdupes.php'...
Removing duplicate Audit requests...
Done.
Applying patch 'phabricator:20141118.diffxaction.sql'...
Applying patch 'phabricator:2014111... (1,836 more bytes) ...

STDERR
(empty) at [<phutil>/src/future/exec/ExecFuture.php:397]
  #0 ExecFuture::resolvex() called at [<phutil>/src/future/exec/execx.php:17]
  #1 execx(string, string, string) called at [<phabricator>/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php:16]
  #2 PhabricatorStorageFixtureScopeGuard::__construct(string) called at [<phabricator>/src/infrastructure/testing/PhabricatorTestCase.php:163]
  #3 PhabricatorTestCase::newStorageFixture() called at [<phabricator>/src/infrastructure/testing/PhabricatorTestCase.php:69]
  #4 PhabricatorTestCase::willRunTestCases(array) called at [<arcanist>/src/unit/engine/PhutilUnitTestEngine.php:53]
  #5 PhutilUnitTestEngine::run() called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:171]
  #6 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:338]

btrahan updated this object.Dec 10 2014, 11:40 PM

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

The important part is probably at the bottom, try this and see if the last few lines are useful?

$ php '/Users/btrahan/Dropbox/code/phalanx/scripts/sql/manage_storage.php' upgrade --force --namespace 'phabricator_unittest_blahblahtesttest'

Oh okay... looks like I have to drop the deprecated column and the deprecated table as part of this diff for arc unit to pass.

15:39:12 ~/Dropbox/code/phalanx (T5604)
~> php '/Users/btrahan/Dropbox/code/phalanx/scripts/sql/manage_storage.php' upgrade --force --namespace 'phabricator_unittest_d6guujbzyuk7ew4hwkmtunu2'
Storage is up to date. Use 'storage status' for details.
Verifying database schemata...
Found no adjustments for schemata.

Target                                                                           Error
phabricator_unittest_d6guujbzyuk7ew4hwkmtunu2_maniphest.maniphest_task.ccPHIDs   Surplus
phabricator_unittest_d6guujbzyuk7ew4hwkmtunu2_maniphest.maniphest_tasksubscriber Surplus

 SCHEMATA ERRORS

The schemata have serious errors (detailed above) which the adjustment
workflow can not fix.

If you are not developing Phabricator itself, report this issue to the
upstream.

If you are developing Phabricator, these errors usually indicate that your
schema specifications do not agree with the schemata your code actually
builds.

btrahan updated this object.Dec 10 2014, 11:45 PM

btrahan edited edge metadata.

add additional sql commands to clear up old data
remove a line of unused code in detail controller

Okay this is ready for review now, including being fully tested. That said, please do let me know if there should be some additional test coverage here.

This looks correct to me, and the test plan seems comprehensive (except for the untested bits). Two possible risk mitigations inline if they seem compelling, not sure where the comfort level shakes out on this one. It looks pretty bulletproof.

resources/sql/autopatches/20141210.maniphestsubscribersmig.2.sql
5	Maybe leave this (and the DAO class for it, so `storage/adjust` doesn't complain) around for a future diff, just in case anything crops up?
src/applications/maniphest/query/ManiphestTaskQuery.php
361	To reduce the risk of this change, consider loading these unconditionally for now (e.g., add `\|\| true`). If the patch is stable, you can then remove the `\|\| true` part later to get the performance benefit. I'm not sure how risky this feels -- I did uncoditional loads for projects (immediately above) because I wasn't confident I could get all/nearly-all of them right easily.

This revision is now accepted and ready to land.Dec 11 2014, 12:00 AM

epriestley awarded a token.Dec 11 2014, 12:00 AM

keep dao class and table for now

i'm going to keep the conditional load for now. its as good as its going to get via code inspection and i should get in a reasonable day tomorrow if any bugs pop up.

Closed by commit rP7d9687057056: Maniphest - use subscribers framework properly (authored by btrahan, committed by btrahan). · Explain WhyDec 11 2014, 12:27 AM

This revision was automatically updated to reflect the committed changes.