Summary: This variable is used before it is defined.
Test Plan: `arc lint`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11434
Summary: Enable strict mode for Javelin when running in NodeJS.
Test Plan: Made sure Aphlict still worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11432
Summary:
In particular, this changes the behavior of NodeJS in the following ways:
- Any attempt to get or modify the global object will result in an error.
- `null` values of `this` will no longer be evaluated to the global object and primitive values of this will not be converted to wrapper objects.
- Writing or deleting properties which have there writeable or configurable attributes set to false will now throw an error instead of failing silently.
- Adding a property to an object whose extensible attribute is false will also throw an error now.
- A functions arguments are not writeable so attempting to change them will now throw an error `arguments = [...]`.
- `with(){}` statements are gone.
- Use of `eval` is effectively banned.
- `eval` and `arguments` are not allowed as variable or function identifiers in any scope.
- The identifiers `implements`, `interface`, `let`, `package`, `private`, `protected`, `public`, `static` and `yield` are all now reserved for future use (roll on ES6).
Test Plan: Verified that Aphlict was still functional.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11430
Summary: Fixes T6963. Long term will likely make this more like other document views, but not worth the time right now since this is only location.
Test Plan: Review Phriction document at desktop and mobile breakpoints. Click menu and see menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6963
Differential Revision: https://secure.phabricator.com/D11420
Summary:
This was broken in D11383. Basically, I had the `ws` module installed globally whilst testing, but the changes made do not work if the `ws` module is installed locally (i.e. in the `./support/aphlict/server/node_modules` directory). After poking around, it seems that this is due to the sandboxing that is done by `JX.require`.
A quick fix is to just //not// use `JX.require`, although you may have a better idea?
The error that is occurring is as follows:
```
<<< UNCAUGHT EXCEPTION! >>>
Error: Cannot find module 'ws'
at Function.Module._resolveFilename (module.js:338:15)
at Function.Module._load (module.js:280:25)
at Module.require (module.js:364:17)
at require (module.js:380:17)
at extra.require (/usr/src/phabricator/webroot/rsrc/externals/javelin/core/init_node.js:48:16)
at /usr/src/phabricator/support/aphlict/server/lib/AphlictClientServer.js:10:17
at Script.(anonymous function) [as runInNewContext] (vm.js:41:22)
at Object.JX.require (/usr/src/phabricator/webroot/rsrc/externals/javelin/core/init_node.js:58:6)
at Object.<anonymous> (/usr/src/phabricator/support/aphlict/server/aphlict_server.js:102:4)
at Module._compile (module.js:456:26)
>>> Server exited!
```
Test Plan: Now able to start the Aphlict server.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: Korvin, epriestley
Maniphest Tasks: T6987
Differential Revision: https://secure.phabricator.com/D11425
Summary: By calling `process.exit(1)` we are forcing the Node.js process to exit prematurely. Specifically, the process is terminated before the error is written to the log file. This can be verified by inspecting the value of `debug._logs[0]._writableState.writing` immediately before the process is terminated.
Test Plan: Ran `./bin/aphlict debug` without the `ws` module being installed. Verified that `<<< UNCAUGHT EXCEPTION! >>>` was echoed to the console as well as the log file. Also verified that the exit status was non-zero.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11426
Summary:
Taking a pass at revamping the edit pages in Projects. Specifically:
- Remove EditMainController
- Move actions from EditMain to Profile
- Move properties from EditMain to Profile
- Move timeline from EditMain to Profile
- Move Open Tasks from Profile to sidenavicon
- Add custom icons and colors to timeline
Feel free to bang on this a bit and give feedback, feels generally correct to me.
Test Plan: Edit everything I could on various projects. Check links, timelines, actions.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11421
Summary: Tidy the Aphlict server by splitting the functionality into two main modules, `AphlictClientServer` and `AphlictAdminServer. There is still further tidying that could be done here, but I feel that this puts us in a much better place.
Test Plan: Sent notifications via `/notification/status/`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11383
Summary:
Ref T6881. Hopefully, this is the hard part.
This adds a new daemon (the "trigger" daemon) which processes triggers, schedules them, and then executes them at the scheduled time. The design is a little complicated, but has these goals:
- High resistance to race conditions: only the application writes to the trigger table; only the daemon writes to the event table. We won't lose events if someone saves a meeting at the same time as we're sending a reminder out for it.
- Execution guarantees: scheduled events are guaranteed to execute exactly once.
- Support for arbitrarily large queues: the daemon will make progress even if there are millions of triggers in queue. The cost to update the queue is proportional to the number of changes in it; the cost to process the queue is proportional to the number of events to execute.
- Relatively good observability: you can monitor the state of the trigger queue reasonably well from the web UI.
- Modular Infrastructure: this is a very low-level construct that Calendar, Phortune, etc., should be able to build on top of.
It doesn't have this stuff yet:
- Not very robust to bad actions: a misbehaving trigger can stop the queue fairly easily. This is OK for now since we aren't planning to make it part of any other applications for a while. We do still get execute-exaclty-once, but it might not happen for a long time (until someone goes and fixes the queue), when we could theoretically continue executing other events.
- Doesn't start automatically: normal users don't need to run this thing yet so I'm not starting it by default.
- Not super well tested: I've vetted the basics but haven't run real workloads through this yet.
- No sophisticated tooling: I added some basic stuff but it's missing some pieces we'll have to build sooner or later, e.g. `bin/trigger cancel` or whatever.
- Intentionally not realtime: This design puts execution guarantees far above realtime concerns, and will not give you precise event execution at 1-second resolution. I think this is the correct goal to pursue architecturally, and certainly correct for subscriptions and meeting reminders. Events which execute after they have become irrelevant can simply decline to do anything (like a meeting reminder which executes after the meeting is over).
In general, the expectation for applications is:
- When creating an object (like a calendar event) that needs to trigger a scheduled action, write a trigger (and save the PHID if you plan to update it later).
- The daemon will process the event and schedule the action efficiently, in a race-free way.
- If you want to move the action, update the trigger and the daemon will take care of it.
- Your action will eventually dump a task into the task queue, and the task daemons will actually perform it.
Test Plan:
Using a test script like this:
```
<?php
require_once 'scripts/__init_script__.php';
$trigger = id(new PhabricatorWorkerTrigger())
->setAction(
new PhabricatorLogTriggerAction(
array(
'message' => 'test',
)))
->setClock(
new PhabricatorMetronomicTriggerClock(
array(
'period' => 33,
)))
->save();
var_dump($trigger);
```
...I queued triggers and ran the daemon:
- Verified triggers fire;
- verified triggers reschedule;
- verified trigger events show up in the web UI;
- tried different periods;
- added some triggers while the daemon was running;
- examined `phd debug` output for anything suspicious.
It seems to work in trivial use case, at least.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11419
Summary:
Ref T6881. This will probably make more sense in a couple of diffs, but this is a class that implements scheduling/recurrence rules. Two rules are provided:
- Trigger an event at a specific time (e.g., a meeting reminder notification).
- Trigger an event on the Nth day of every month (e.g., a subscription bill).
At some point, we'll presumably add a rule for T2896 (maybe using the "RRULE" spec) so you can do stuff like "the second to last thursday of every month", etc., but we don't need that for now.
(The "Nth day of every month, or move it back if no such day exists" rule doesn't seem to be expressible with the "RRULE" format, so implementing that wouldn't give us a superset of this. I think this rule is correct and desirable for this purpose, though.)
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11403
Summary:
This is unusual, but if `getWorkerInstance()` throws we end up with an undefined `$worker` when recovering from the exception.
Instead, handle this case slightly more gracefully.
The easiest way to hit this is to schedule a task for a worker that doesn't exist (or remove an existing worker, which is what I did to hit it).
Test Plan: Saw a more graceful error recovery; ran some normal successful tasks out of the queue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11413
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `PhutilArgumentWorkflow::__construct`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11415
Summary: Ref T6822. This method needs to be `public` because it is called from `PhabricatorApplicationSearchController::buildApplicationMenu()`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11416
Summary: Fixes T5344. Essentially, we only make the AJAX request to `/notification/individual/` if we are the leader tab (i.e. only one tab will make this request). Once a response has been received from the server (containing the contents of the notification), we broadcast the message contents back to all other tabs for rendering.
Test Plan:
Opened two tabs on `/notification/status/` and clicked "Send Test Notification".
**Before**
```lang=bash, name=tail -f /var/log/phabricator-access.log | grep /notification/individual/
[Tue, 13 Jan 2015 20:10:37 +1100] 17033 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 236036
[Tue, 13 Jan 2015 20:10:37 +1100] 17657 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 24130
```
**After**
```lang=bash, name=tail -f /var/log/phabricator-access.log | grep /notification/individual/
[Tue, 13 Jan 2015 20:11:15 +1100] 17657 phabricator 10.0.0.1 josh PhabricatorNotificationIndividualController - /notification/individual/-200 180217
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5344
Differential Revision: https://secure.phabricator.com/D11360
Summary: Ref T6822. This method is only called from `PhutilDaemon::execute()` and can be made `protected`.
Test Plan: See D11404.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11405
Summary: Ref T6822. This method is only called from within the `PhabricatorWorker::executeTask()` and `PhabricatorWorker::scheduleTask()` methods.
Test Plan: `grep`ped for `->doWork`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11406
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from `LiskDAO::update()`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11409
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `LiskDAO::establishConnection()`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11412
Summary: Ref T6944. This was not quite implemented correctly in D11387.
Test Plan: Saw no more exceptions about being unable to create `/var/log`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: maxhodak, Korvin, epriestley
Maniphest Tasks: T6944
Differential Revision: https://secure.phabricator.com/D11397
Summary: ...also adds policies on who can view and who can edit an action. Fixes T6949.
Test Plan: viewed a secret through the new UI and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6949
Differential Revision: https://secure.phabricator.com/D11401
Summary: Ref T6962. Mainly accomplished by re-factoring the base editor `buildMailBody` function and then using it differently in the `DifferentialTransactionEditor`.
Test Plan: commented on a revision leaving inline feedback. inspected via bin/mail and it looked good! also made a maniphest comment and checked that email, which still looked good.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6962
Differential Revision: https://secure.phabricator.com/D11402
Summary: Fixes T6959.
Test Plan: When I was ready to test the feature, the "Daemon & Web config" error already showed up, from having added phd.variant-config. I went meta and changed the value of phd.variant-config to have phd.variant-config. The config error disappeared. I then changed the conpherence setting about conpherence email prefix and the error showed up again. Removing the conpherence config setting made the error disappear once more.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6959
Differential Revision: https://secure.phabricator.com/D11399
Summary: Just some housekeeping... mostly just removing some unused variables.
Test Plan: Checked that I was still about to receive notifications from `/notification/status/`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11398
Summary:
Ref T6881. This is part 1 of my 35-step plan to support subscriptions that bill monthly.
Expanding the capabilities of counters will let me use them to create a logical clock on time-based event updates, build a daemon on top of that, and eventually get time-based triggers.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T6881
Differential Revision: https://secure.phabricator.com/D11395
Summary:
For block-level elements that have a margin-top or margin-bottom set
(generally to 12px), also reset the appropriate margin to 0 when
they're a first-child or last-child of their parents.
The change doesn't affect nested lists, their selector is more specific.
Test Plan:
Look at some comments or wiki documents that end with different
block elements, verify that the margins are pretty.
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: #blessed_reviewers, chad
Subscribers: Korvin, epriestley
Projects: #remarkup
Maniphest Tasks: T6968
Differential Revision: https://secure.phabricator.com/D11382
Summary: Fixes T6842. Allow the daemons to always be terminated, even if MySQL is down. I was hoping to be able to optionally enable this behavior with the `--force` flag, but this seems messy.
Test Plan:
```lang=bash
> ./bin/phd start
Freeing active task leases...
Freed 1 task lease(s).
Preparing to launch daemons.
NOTE: Logs will appear in '/var/tmp/phd/log/daemons.log'.
Starting daemons as phd
Launching daemon "PhabricatorRepositoryPullLocalDaemon".
Starting daemons as phd
Launching daemon "PhabricatorGarbageCollectorDaemon".
Starting daemons as phd
Launching daemon "PhabricatorTaskmasterDaemon".
Done.
> service mysql stop
mysql stop/waiting
> ./bin/phd stop
Interrupting daemon 'PhabricatorRepositoryPullLocalDaemon' (4263)...
Interrupting daemon 'PhabricatorGarbageCollectorDaemon' (4271)...
Interrupting daemon 'PhabricatorTaskmasterDaemon' (4287)...
Daemon 4263 exited.
Daemon 4271 exited.
Daemon 4287 exited.
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6842
Differential Revision: https://secure.phabricator.com/D11385
Summary: Fixes T6944. Attempt to automatically create the log directory for the Aphlict server. If the directory can't be created, throw a helpful exception.
Test Plan:
# Set `notification.log` to `/var/log/aphlict/aphlict.log`.
# Ran `./bin/aphlict debug` and saw an exception (because the user doesn't have permissions to create the `/var/log/aphlict` directory).
# Ran `sudo chmod 777 /var/log`.
# Ran `./bin/aphlict debug` and saw the `/var/log/aphlict` directory created.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6944
Differential Revision: https://secure.phabricator.com/D11387
Summary: I assume we've shown this long enough, plus with redesign it's a good time to remove.
Test Plan: reload page, no link
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11394
Summary: We still seem to reach for this, though may be time to remove Wiki?
Test Plan: view link
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11393
Summary: In Maniphest, we provide an additional caption shortcut if you can create projects, which has no use if you cant. Fixes T6969
Test Plan: Check page with and without a user's capability.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6969
Differential Revision: https://secure.phabricator.com/D11390
Summary: Ref T6971. This fixes the error the user reported. Not sure what's up with the root cause of their issue.
Test Plan: Went to `/auth/config/new/asdfqwer/` and got a 404 instead of an exception.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6971
Differential Revision: https://secure.phabricator.com/D11388
Summary: Fixes T6955.
Test Plan: made an oauth app. made a test authorization. ran bin/remove destroy <phid of oauth client> and there were no errors. verified oauth app and test authorization were both gone.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6955
Differential Revision: https://secure.phabricator.com/D11378
Summary: Fixes T6957. If / when a policy object is destroyed, access to an object that uses that policy object is denied.
Test Plan: looked around in the code to fail confident enough to write the summary above
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6957
Differential Revision: https://secure.phabricator.com/D11380
Summary:
Fixes T6548.
- This workflow doesn't work under reasonable configurations and isn't trivial to fix (see T6548).
- We don't need it; this just makes things a little bit faster if you have to migrate everything (e.g., immediately after T1191) and the installs we know about have generally upgraded by now.
- This keeps kicking PKCS8 keys out of cache which is a pain.
Test Plan: Ran `bin/storage adjust` without it doing an implicit cache purge.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6548
Differential Revision: https://secure.phabricator.com/D11377
Summary: Fixes T6964, makes action links float instead of absolutely positioned.
Test Plan: Tested UIExamples, actions in single line headers, multi line headers, headers with images, workboard headers. Test desktop, mobile, and tablet breakpoints. Long titles wrap as expected as button list grows.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6964
Differential Revision: https://secure.phabricator.com/D11379
Summary: Fixes T6953. As discussed, this directory //should// be linted.
Test Plan: Ran `arc lint` on a file in `webroot/rsrc/externals/javelin` and saw linter errors.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6953
Differential Revision: https://secure.phabricator.com/D11375
Summary: We could still miss this if the policy had never been customized and we returned early after one of the other checks.
Test Plan:
Works great on instances now.
{F267067}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11374
Summary: What do you think this is, PHP?
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11361
Summary: Third time lucky... the filename should match the class name now.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11362
Summary: Ref T6822. There are a bunch of places where we call `$something->generatePHID(...)` externally (outside of the class). Therefore, these methods need to be `public`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11363
