Page MenuHomePhabricator

Added a `token_token` table in anticipation of some data-driven tokens
ClosedPublic

Authored by jcox on Sep 28 2016, 2:41 PM.

Details

Summary

Ref T11217. This just adds the table that we'll store tokens in. It doesn't make use of the table at all yet. This is mostly pulled from this diff (D16178). Specifically I mostly followed Evan's instructions related to the token table here: D16178#189120.

Test Plan

I ran ./bin/storage upgrade successfully and there were no schema errors.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jcox updated this revision to Diff 40012.Sep 28 2016, 2:41 PM
jcox retitled this revision from to Added a `token_token` table in anticipation of some data-driven tokens.
jcox updated this object.
jcox edited the test plan for this revision. (Show Details)
epriestley accepted this revision.Sep 28 2016, 2:56 PM
epriestley added a reviewer: epriestley.

Couple of possible things inline, but this looks pretty good to me.

resources/sql/autopatches/20160928.tokentoken.sql
12

This should maybe be nullable since I think builtin tokens probably won't use an actual File for their data (we can just read it directly off disk) but we can fix that easily later and I'm not 100% sure it will actually be how things turn out.

src/applications/tokens/storage/PhabricatorTokensToken.php
25–27

(Column doesn't exist?)

98–100

Let's just omit this for now -- we don't have a mailKey, and I think interacting with tokens via email seems a little silly / not worth building, at least for now. I could be wrong and maybe emailing datboi@whatever.phabricator.com will be all the rage in a few months, but doesn't feel like the highest-value interaction here.

112–117

I think this isn't right -- it's loading copies of itself by a nonexistent column. Probably intended to delete PhabricatorTokenGiven instead (replace new self() with new PhabricatorTokenGiven())?

I think we could reasonably delete the File by filePHID, if filePHID is set too.

126

This is ultimately a product question but maybe creating a token shouldn't permanently auto-subscribe you to it. We don't really have a hard-and-fast rule for this but I think the soft rule is "if you own the object", and a creator's ownership claim on a token feels a little tenuous (e.g., compared to a Differential Revision, which is clearly strongly-owned).

We recently backed off on auto-subscribe on Phame blogs, and this ownership feels similarly soft to me.

We can easily tweak this later, though.

This revision is now accepted and ready to land.Sep 28 2016, 2:56 PM
jcox marked 4 inline comments as done.Sep 28 2016, 3:13 PM
jcox added inline comments.
src/applications/tokens/storage/PhabricatorTokensToken.php
126

What does it even mean to be subscribed to a token? I can't think of a scenario where I would want that.

jcox updated this revision to Diff 40014.Sep 28 2016, 3:18 PM
jcox edited edge metadata.

Fixed issues brought up in CR

jcox updated this revision to Diff 40015.Sep 28 2016, 3:20 PM

Gonna go ahead and say no one should be automatically subscribed to tokens

This revision was automatically updated to reflect the committed changes.