Summary: Fixes T8518. Defaulting $events to null creates carnage further down the stack. Instead, default it to array() so foreach, array_select_keys, etc. don't barf.
Test Plan: logic and will ask the reporting user to verify since they can repro 100%
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8518
Differential Revision: https://secure.phabricator.com/D13314
Summary:
Fixes T8548. This needs to be cleaned up more generally at some point, but stop the bleeding for now.
If a thread member is invited to an event with a non-thread-member host, we try to render the host but don't have their handle (this is a holdover from the bygone days of events as statuses).
For now, just don't render anything. In the future, it might be nice to render (some of?) the attendees who are thread members, but I suspect we may revisit this widget more generally.
Test Plan:
- As a non thread-member, created an event and invited a thread member.
- Viewed thread as thread-member.
- Saw widget fatal.
- Applied patch.
- As thread-member, saw widget render with just "9AM-10AM", instead of "host, 9AM-10AM".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8548
Differential Revision: https://secure.phabricator.com/D13299
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary:
Ref T8488. Ref T5681.
Now you can just use `id(new ConpherenceThreadMembersPolicyRule())->getObjectPolicyFullKey()` as a policy.
Added tests for TaskAuthor + ThreadMembers.
Test Plan:
- Ran tests.
- Set a thread policy to "Members of thread'.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681, T8488
Differential Revision: https://secure.phabricator.com/D13258
Summary: Ref T8478. This is easier than I thought, let's see if it's conspicious?
Test Plan:
- Embedded `{Z2}` and saw "Private Correspondence".
- Still saw normal stuff with useful participant lists/titles in all main UIs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8478
Differential Revision: https://secure.phabricator.com/D13220
Summary: Remove the `*TransactionType` classes and define the constants in the corresponding `*Transaction` class instead.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13188
Summary:
Ref T6367. Removes `multiplexMail()`!
We can't pass a single body into a function which splits it anymore: we need to split recipients first, then build bodies for each recipient list. This lets us build separate bodies for each recipient's individual translation/access levels.
The new logic does this:
- First, split recipients into groups called "targets".
- Each target corresponds to one actual mail we're going to build.
- Each target has a viewer (whose translation / access levels will be used to generate the mail).
- Each target has a to/cc list (the users who we'll ultimately send the mail to).
- For each target, build a custom mail body based on the viewer's access levels and settings (language prefs not actually implemented).
- Then, deliver the mail.
Test Plan:
- Read new config help.
Then did a bunch of testing, primarily with `bin/mail list-outbound` and `bin/mail show-outbound` (to review generated mail), `bin/phd debug taskmaster` (to run daemons freely) and `bin/worker execute --id <id>` (to repeatedly test a specific piece of code after identifying an issue).
With `one-mail-per-recipient` on (default):
- Sent mail to multiple users.
- Verified mail showed up in `mail list-outbound`.
- Examined mail with `mail show-outbound`.
- Added a project that a subscriber could not see.
- Verified it was not present in `X-Phabricator-Projects`.
- Verified it was rendered as "Restricted Project" for the non-permissioned viewer.
- Added a subscriber, then changed the object policy so they could not see it and sent mail.
- Verified I received mail but the other user did not.
- Enabled public replies and verified mail generated with public addresses.
- Disabld public replies and verified mail generated with private addresses.
With `one-mail-per-recipient` off:
- Verified that one mail is sent to all recipients.
- Verified users who can not see the object are still filtered.
- Verified that partially-visible projects are completely visible in the mail (this violates policies, as documented, as the best available compromise).
- Enabled public replies and verified the mail generated with "Reply To".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: carlsverre, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13131
Summary: Fixes T8329. I was able to figure out a reasonable way to have the full conpherence default to the email settings panel. I think this is cleaner than making things a dialogue as I rambled about in the description for T8329.
Test Plan: using /bin/mail to verify correct email links were generated for conpherence notifications and maniphest (general) notifications.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8329
Differential Revision: https://secure.phabricator.com/D13058
Summary:
Fixes T8154. Now you can tell when a Conpherence was started by mousing over the date. I think this is sufficient.
This ends up cleaning up the code significantly; it used to inherit from some transaction view class and had built up lots of cludge over time. Much cleaner now.
Test Plan: moused over tooltips in both durable column and full conpherence and saw correct full dates. clicked on a timestamp in durable column and went to right place in full conpherence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8154
Differential Revision: https://secure.phabricator.com/D13057
Summary: Fixes T8251. I think this used to be "lucky handle" when it was all one giant terrible function, so restore previous functionality and have the title be the viewer's name in this case.
Test Plan: rocked a solo conpherence and saw my username as the title
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8251
Differential Revision: https://secure.phabricator.com/D12935
Summary: Ref T6403. Conpherence keeps track of comments for message counts so we needed some special attention there. Otherwise, straight-forward.
Test Plan: left a comment on a diff with inline comments. sent messages in conpherence successfully. verified unread count incremented correctly for sent messages for users.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6403
Differential Revision: https://secure.phabricator.com/D12932
Summary: Fixes T8217. Broken circa D12760. Some browsers rendered nothing while others rendered a ",".
Test Plan: added folks to conpherences and saw people widget update correctly. added files to conpherence and saw file widget update correctly. Tested on Chrome and Firefox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8217
Differential Revision: https://secure.phabricator.com/D12912
Summary:
Ref T8183. See that task for discussion.
- For now, events always mark users as "Away".
- In the future, we may reintroduce "sporradic" or other more complicated availability states, but they would be properties of the invitee, not of the event itself.
- This also removes the long-deprecated `user.addstatus` and `user.removestatus` Conduit calls.
Test Plan:
- Created, edited, viewed events.
- Grepped for removed symbols.
- Viewed profile calendar.
- Viewed Conpherence calendar.
- Load Conduit console.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8183
Differential Revision: https://secure.phabricator.com/D12840
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Fixes T6713. Before this diff, we would update the DOM when various requests came back, but the logic to erase race conditions proved too tricky for me to get right. Instead, change the algorithm up and keep a set of transaction ids around per thread. When its time to update the transactions, sort the list of ids and just render the whole darn set again.
To make this work, this ends up adding transacton ids to fake transactons like "show older" and date markers. This is able to work by using a float sort and giving these transactions ids that are .5 from being an integer and in the right place numerically.
Test Plan: for durable column, clicked show older and it worked. sent a message and it worked. for main view, clicked show older and it worked. sent a message and it worked.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6713
Differential Revision: https://secure.phabricator.com/D12819
Summary:
Ref T7708.
This changes things to $viewer->loadHandles where applicable in the durable column render stack. I saw some big wins on my test data like 34 queries => 24 queries on a newly created room as my default thread.
For my test data, the next big perf win would be to change how remarkup rendering works and try to multiload all objects of a certain type in one shot.
e.g. `PhabricatorEmbedFileRemarkupRule` implements `loadObjects` as do all classes which inherit from `PhabricatorObjectRemarkupRule`. This is because `PhabricatorObjectRemarkupRule` implements its `didMarkupText` method using `loadObjects`, and `didMarkupText` gets called per transaction over in `PhabricatorMarkupEngine->process()`. Instead, the `loadObjects` in `didMarkupText` should be hitting some cache, and we should do a bulk load for all `PhabricatorEmbedFileRemarkupRule` that had matches earlier in the rendering stack. ...I think.
Test Plan: carefully looked at "Services" tab in dark console and noted fewer queries with changes post changes versus pre changes
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7708
Differential Revision: https://secure.phabricator.com/D12780
Summary: Fixes T8102. This makes public rooms actually work. Also lets users see the search listings page so they can wander into all public rooms without logging in.
Test Plan: As logged out user, visited ZXX and ZYY. ZXX was public, so I could see it and had a little "Login to Participate" button in the bottom. ZYY was not public so I was prompted to login. Back on ZXX I clicked the Conpherence crumb and got a sensible UI where most links prompted me to login. CLicked "search" and saw listings for all public rooms.
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8102
Differential Revision: https://secure.phabricator.com/D12778
Summary:
Ref T3165. This:
- Fixes a bug with overlapping matches.
- Makes the UI a little less hideous (and more standard).
- Links comments into the chat history view.
Test Plan: {F396749}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3165
Differential Revision: https://secure.phabricator.com/D12777
Summary: Ref T7708. Rather than invoking the general client -> server dropdown refresh path, return the data with the various conpherence requests and update the dropdowns that way. Saves 2 client -> server requests per conpherence action.
Test Plan: loaded up /conpherence/ and noted message count deduct correctly. clicked specific message and noted message count deduct successfully. did same two tests via durable column and again saw message counts deduct successfully.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7708
Differential Revision: https://secure.phabricator.com/D12761
Summary: Ref T7708. We were generating things like the files widget when users sent a comment. This is unnecessary if we are in minimal display mode. This saves us fetching some data + rendering.
Test Plan: sent messages successfully in durable column and full conpherence view
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7708
Differential Revision: https://secure.phabricator.com/D12760
Summary: Fixes T8066. These search results were using getDisplayData but only need a title. Use more granular API introduced in rPf311f3f910e31b72345e37bf2e5bea0f278fd111.
Test Plan: ran a conpherence query for messages and no more fatal
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8066
Differential Revision: https://secure.phabricator.com/D12719
Summary: Fixes T7254. This reverts the previous functionality, but makes pertinent updates like scaling the images to 35 x 35. Codebase had moved on quite a bit so far from a straight revert but nothing too tricky relative to the code that was here before. This does not allow for changing the images from the conpherence durable column view -- that would require some JS trickery, but also doesn't fit into the current notion of the column being "light". Can always modify this later.
Test Plan:
- from full conpherence, uploaded a square pic and things looked nice
- from full conpherence, uploaded a rectangular pic and wasnt happy, so reinvoked edit dialog and used crop control to make it better
- noted could not update picture from conpherence durable column
- used different user and noted could see custom picture
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: CodeMouse92, Korvin, epriestley
Maniphest Tasks: T7254
Differential Revision: https://secure.phabricator.com/D12648
Summary: Fixes T7757. Since anchor links can't be processed server side, we have to detect the message is old in javascript, then re-loaded the page. This opens up a new corner case where we have to paginate in newer messages, so this also adds support for that.
Test Plan:
- set main query limit to 8 and then visited ZXX#YYY. noted a second quick load of YYY, that YYY ended up highlighted and scrolled to.
- used "show newer messages" and "show older messages" successfully, taking care to make sure transaction ids were all correct with no off by one errors, etc.
- opened and closed durable column to make sure that still works too
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7757
Differential Revision: https://secure.phabricator.com/D12633
Summary: Fixes T7439. Also two callsites I missed earlier.
Test Plan: Tried to search for strings like `'E` and `"E`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7439
Differential Revision: https://secure.phabricator.com/D12616
Summary: Ref T3165. This is pretty awful looking, but should pull the correct data.
Test Plan: {F387567}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3165
Differential Revision: https://secure.phabricator.com/D12589
Summary: Ref T3165. This needs more work but does the basics.
Test Plan: {F387514}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3165
Differential Revision: https://secure.phabricator.com/D12588
Summary:
- Now that we have "browse", this is a much more reasonable control for random sets of things.
- The new explicit search scope selector reduces the need to fiddle with this field manually, too.
Test Plan:
{F379292}
{F379293}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12510
Summary: Fixes T7857. Some sorting gymnastics - make a nice list for natcasesort and then array_select_key to sort - but nothing too crazy.
Test Plan: made a room with user "btrahan" and other users "BTRAHA", "BTRAHANCAPS", and "xerxes". chat room correctly ordered "btrahan" (since logged in user always at top), "BTRAHA", "BTRAHANCAPS", "xerxes". Viewed room as "xerxes" and saw ordering "xerxes", "BTRAHA", "btrahan", "BTRAHANCAPS".
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7857
Differential Revision: https://secure.phabricator.com/D12507
Summary: Ref D12448. Ref T7573. This changes quicksand up a bit so rather than caching just rendered HTML we also cache the initial response from the server. We also fire off a quicksand-redraw event which will let things like the page objects for notifications update correctly while using Quicksand (see D12448).
Test Plan: loaded up /p/btrahan/ Clicked the UI elements to navigate to various profile views up to maniphest. clicked back until back at /p/btrahan/ and it worked. clicked forward until all the way back to maniphest and it worked. clicked back 2x, then clicked new links, then back and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7573
Differential Revision: https://secure.phabricator.com/D12449
Summary:
Ref T4100. Ref T5595. This allows PolicyAwareQuery to write all the logic for AND, OR, NOT, and NULL (i.e., "not in any projects") queries against any edge type.
It accepts an edge type and a list of constraints (which are basically just operator-value pairs, like `<NOT, PHID-X-Y>`, meaning the results must not have an edge connecting them to `PHID-X-Y`).
This doesn't actually do anything yet; see future diffs.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12455
Summary:
Ref T4100. Ref T5595. These functions are trivial for now, but move us toward being able to define more default query behavior by default.
Future changes will give these methods meaningful, nontrivial behaviors.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5595, T4100
Differential Revision: https://secure.phabricator.com/D12454
Summary:
Ref T4100. Ref T5595.
To support a unified "Projects:" query across all applications, a future diff is going to add a set of "Edge Logic" capabilities to `PolicyAwareQuery` which write the required SELECT, JOIN, WHERE, HAVING and GROUP clauses for you.
With the addition of "Edge Logic", we'll have three systems which may need to build components of query claues: ordering/paging, customfields/applicationsearch, and edge logic.
For most clauses, queries don't currently call into the parent explicitly to get default components. I want to move more query construction logic up the class tree so it can be shared.
For most methods, this isn't a problem, but many subclasses define a `buildWhereClause()`. Make all such definitions protected and consistent.
This causes no behavioral changes.
Test Plan: Ran `arc unit --everything`, which does a pretty through job of verifying this statically.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, hach-que, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12453
Summary: Fixes T7840. Add some unit tests. One explicitly covers the case in T7840. Other cases are "base" cases for rooms and threads, plus the version if T7840 for a thread, where a policy exception should be thrown.
Test Plan: `arc unit` and was able to successfully add a file to a conpherence room i did not have edit permissions on
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7840
Differential Revision: https://secure.phabricator.com/D12443
Summary: Follow on to D12410. This kills the general CSS file that once powered application transaction in favor of a specific CSS file for the shared transaction styles for both full view and durable column view. I was able to delete some stuff and identify some shared stuff but there is probably more to go here. Also, rename "phabricator-x" to "conpherence-x" for class names.
Test Plan: viewed full conpherence and durable column conpherence and things looked nice
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12412
Summary:
D12409 made me realize this was a bit janky. `PhabricatorTransactionView` was only being used by Conpherence, so move and rename that class to `ConpherenceTransactionView`. Also, rename the existing `ConpherenceTransactionView` to `ConpherenceTransactionRenderer`, moving the actual view bits into the new `ConpherenceTransactionView`. Resulting code is a bit cleaner IMO.
Diff 1 of 2 (second diff has to be written. =D). Diff 2 will take care of the CSS and possibly clean things up further.
Test Plan: played around in conpherence full and conpherence column and things looked nice
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12410
Summary: Ref T7757. This diff made me realize that `PhabricatorTransactionView` is only used in Conpherence, as well as that `ConpherenceTransactionView` is more like a rendering class with a few static methods. Going to take a diff after this to clean all that up.
Test Plan: opened up durable column, clicked on timestamp, voila!
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7757
Differential Revision: https://secure.phabricator.com/D12409
Summary: Fixes T7756. This is the last little stray bit, though finishing T7757 also helps this feature IMO.
Test Plan: said "ZXX is the best" in comment on DXX and saw proper mention transaction on ZXX
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7756
Differential Revision: https://secure.phabricator.com/D12405
Summary:
Ref T7757. Oddities include:
- not working in column view, since the generic anchor technology conflicts once you navigate to a page with a transaction timeline view
- not working if you are linking to a message not included in initial load
Remaining work is addressing these oddities.
- make column view timestamp link to full conpherence correctly?
- make back end load from hyperlinked transaction forward? or do it more like application transactions and have the client keep requesting stuff until it gets it?
Open to suggestions! :D
Test Plan: played around in conpherence full and stuff looked okay. noted no changes as intended in column view.
Reviewers: epriestley, chad
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7757
Differential Revision: https://secure.phabricator.com/D12402
Summary: Ref T7756. Now viewing individual threads in Conpherence is `ZXXX` driven. Also adds remarkup support.
Test Plan: clicked around on list of conpherences in full view and it worked. selected 'view in conpherence' action from column and loaded correct `ZXXX` uri. Typed `ZXXX` in Maniphest and saw it link to Conpherence room.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7756
Differential Revision: https://secure.phabricator.com/D12397
Summary: Turns out the pertinent views don't use this data anyway. Additonally, change the remaining pathway to fetch 15% of the rows it fetched in D12347. Fixes T7815.
Test Plan: clicked around and things worked. my instance always feels snappy though
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7815
Differential Revision: https://secure.phabricator.com/D12399
Summary: Ref T7815. Pretty sure this is right? Seems OK locally. Also: simpler!
Test Plan:
- Conpherence is right-side-up.
- Transactions on other objects are also still right-side-up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7815
Differential Revision: https://secure.phabricator.com/D12398
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.
Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.
Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.
Test Plan:
- Ran `arc unit --everything`.
- Called `conduit.query`.
- Browsed Conduit UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T5873, T7803
Differential Revision: https://secure.phabricator.com/D12380
Summary: Ref T7803. Remove these in favor of more generalized paging and ordering.
Test Plan: Sorted and paged results in various applications.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12378
Summary:
For the price of loading transactions more consistently, we get a better subtitle. We do this in all cases EXCEPT for when we're grabbing handles, because that makes the handles pretty heavy weight and I could even feel the perf hit on my development machine and we don't use subtitle there anyway. We may want to cache the latest message on the conpherence thread object to improve performance here as well as consider falling back to "A, B, C..." more often. Code is written such that no transactions means an automagical fallback.
Fixes T7795. (Technically, there's still a note about handle code conversion work on T7795 but we'll get that generally later.)
Test Plan:
played around with conpherence in both views and things seemed to work nicely.
made sure to try the original repro in T7795 and couldn't get that to go either
posted a long comment and verified that the CSS / string truncation both make it display nicely. Note that without the CSS the chosen glyph value can be too high to fit nicely at times.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7795
Differential Revision: https://secure.phabricator.com/D12347
