Summary:
Depends on D20291. Ref T13259. Move all the simple cases (where paging depends only on the partial object and does not depend on keys) to a simple wrapper.
This leaves a smaller set of more complex cases where we care about external data or which keys were requested that I'll convert in followups.
Test Plan: Poked at things, but a lot of stuff is still broken until everything is converted.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20292
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary:
Fixes T7477. Fixes T13066. Currently, inbound mail is processed by the first receiver that matches any "To:" address. "Cc" addresses are ignored.
**To, CC, and Multiple Receivers**
Some users would like to be able to "Cc" addresses like `bugs@` instead of having to "To" the address, which makes perfect sense. That's the driving use case behind T7477.
Since users can To/Cc multiple "create object" or "update object" addresses, I also wanted to make the behavior more general. For example, if you email `bugs@` and also `paste@`, your mail might reasonably make both a Task and a Paste. Is this useful? I'm not sure. But it seems like it's pretty clearly the best match for user intent, and the least-surprising behavior we can have. There's also no good rule for picking which address "wins" when two or more match -- we ended up with "address order", which is pretty arbitrary since "To" and "Cc" are not really ordered fields.
One part of this change is removing `phabricator.allow-email-users`. In practice, this option only controlled whether users were allowed to send mail to "Application Email" addresses with a configured default author, and it's unlikely that we'll expand it since I think the future of external/grey users is Nuance, not richer interaction with Maniphest/Differential/etc. Since this option only made "Default Author" work and "Default Author" is optional, we can simplify behavior by making the rule work like this:
- If an address specifies a default author, it allows public email.
- If an address does not, it doesn't.
That's basically how it worked already, except that you could intentionally "break" the behavior by not configuring `phabricator.allow-email-users`. This is a backwards compatility change with possible security implications (it might allow email in that was previously blocked by configuration) that I'll call out in the changelog, but I suspect that no installs are really impacted and this new behavior is generally more intuitive.
A somewhat related change here is that each receiver is allowed to react to each individual email address, instead of firing once. This allows you to configure `bugs-a@` and `bugs-b@` and CC them both and get two tasks. Useful? Maybe not, but seems like the best execution of intent.
**Sender vs Author**
Adjacently, T13066 described an improvement to error handling behavior here: we did not distinguish between "sender" (the user matching the email "From" address) and "actor" (the user we're actually acting as in the application). These are different when you're some internet rando and send to `bugs@`, which has a default author. Then the "sender" is `null` and the "author" is `@bugs-robot` or whatever (some user account you've configured).
This refines "Sender" vs "Author". This is mostly a purity/correctness change, but it means that we won't send random email error messages to `@bugs-robot`.
Since receivers are now allowed to process mail with no "sender" if they have some default "actor" they would rather use instead, it's not an error to send from an invalid address unless nothing processes the mail.
**Other**
This removes the "abundant receivers" error since this is no longer an error.
This always sets "external user" mail recipients to be unverified. As far as I can tell, there's no pathway by which we send them email anyway (before or after this change), although it's possible I'm missing something somewhere.
Test Plan:
I did most of this with `bin/mail receive-test`. I rigged the workflow slightly for some of it since it doesn't support multiple addresses or explicit "CC" and adding either would be a bit tricky.
These could also be tested with `scripts/mail/mail_handler.php`, but I don't currently have the MIME parser extension installed locally after a recent upgrade to Mojave and suspect T13232 makes it tricky to install.
- Ran unit tests, which provide significant coverage of this flow.
- Sent mail to multiple Maniphest application emails, got multiple tasks.
- Sent mail to a Maniphest and a Paste application email, got a task and a paste.
- Sent mail to a task.
- Saw original email recorded on tasks. This is a behavior particular to tasks.
- Sent mail to a paste.
- Sent mail to a mock.
- Sent mail to a Phame blog post.
- Sent mail to a Legalpad document.
- Sent mail to a Conpherence thread.
- Sent mail to a poll.
- This isn't every type of supported object but it's enough of them that I'm pretty confident I didn't break the whole flow.
- Sent mail to an object I could not view (got an error).
- As a non-user, sent mail to several "create an object..." addresses.
- Addresses with a default user worked (e.g., created a task).
- Addresses without a default user did not work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13066, T7477
Differential Revision: https://secure.phabricator.com/D19952
Summary:
Depends on D19953. Ref T9141. We have a "MetaMTAAttachment" object, rename it to "MailAttachment".
Also add a "Header" object and an "EmailMessage" object. Currently, mail adapters have a large number of methods like `setSubject()`, `addTo()`, etc, that I would like to remove.
I'd like the API to be more like `sendMessage(PhabricatorMailExternalMessage $message)`. This is likely a significant simplification anyway, since the implementations of all these methods are just copy/pasted boilerplate anyway (lots of `$this->subject = $subject;`) and this will let Adapters support other message media (SMS, APNS, Whatsapp, etc.)
That's a larger change, but move toward a world where we can build a concrete `$message` object for "email" or "sms".
The `PhabricatorMailEmailMessage` object is just a dumb, flat object representation of the information an adapter needs to actually send mail. The existing `PhabricatorMetaMTAMail` is a much more complex object and has a lot of rich data (delivery status, related object PHIDs, etc) and is a storage object.
The new flow will be something like:
- `PhabricatorMetaMTAMail` (possibly renamed) is the storage object for any outbound message on this channel. It tracks message content, acceptable delivery media (SMS vs email), delivery status, related objects, has a PHID, and has a daemon worker associated with delivering it.
- It builds a `PhabricatorMailExternalMessage`, which is a simple, flat description of the message it wants to send. The subclass of this object depends on the message medium. For email, this will be an `EmailMessage`. This is just a "bag of strings" sort of object, with appropriate flattened values for the adapter to work with (e.g., Email has email addresses, SMS has phone numbers).
- It passes the `ExternalMessage` (which is a `MailMessage` or `SMSMessage` or whatever) to the adapter.
- The adapter reads the nice flat properties off it and turns them into an API request, SMTP call, etc.
This is sort of how things work today anyway. The major change is that I want to hand off a "bag of strings" object instead of calling `setX()`, `setY()`, `setZ()` with each individual value.
Test Plan: Grepped for `MetaMTAAttachment`. This doesn't change any behavior yet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T9141
Differential Revision: https://secure.phabricator.com/D19954
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary:
Ref T13222. See PHI683. Currently, you can "Change subtype..." via Conduit and the bulk editor, but not via the comment action stack or edit forms.
In PHI683 an install is doing this often enough that they'd like it to become a first-class action. I've generally been cautious about pushing this action to become a first-class action (there are some inevitable rough edges and I don't want to add too much complexity if there isn't a use case for it) but since we have evidence that users would find it useful and nothing has exploded yet, I'm comfortable taking another step forward.
Currently, `EditEngine` has this sort of weird `setIsConduitOnly()` method. This actually means more like "this doesn't show up on forms". Make it better align with that. In particular, a "conduit only" field can already show up in the bulk editor, which is goofy. Change this to `setIsFormField()` and convert/simplify existing callsites.
Test Plan:
There are a lot of ways to reach EditEngine so this probably isn't entirely exhaustive, but I think I got pretty much anything which is likely to break:
- Searched for `setIsConduitOnly()` and `getIsConduitOnly()`, converted all callsites to `setIsFormField()`.
- Searched for `setIsLockable()`, `setIsReorderable()` and `setIsDefaultable()` and aligned these calls to intent where applicable.
- Created an Almanac binding.
- Edited an Almanac binding.
- Created an Almanac service.
- Edited an Almanac service.
- Edited a binding property.
- Deleted a binding property.
- Created and edited a badge.
- Awarded and revoked a badge.
- Created and edited an event.
- Made an event recurring.
- Created and edited a Conpherence thread.
- Edited and updated the diff for a revision.
- Created and edited a repository.
- Created and disabled repository URIs.
- Created and edited a blueprint.
- Created and edited tasks.
- Created a paste, edited/archived a paste.
- Created/edited/archived a package.
- Created/edited a project.
- Made comments.
- Moved tasks on workboards via comment action stack.
- Changed task subtype via comment action stack.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19842
Summary: Depends on D19810. Ref T13217. Ref T13216. I mostly used `grep implode | grep OR` and `grep implode | grep AND` to find these -- not totally exhaustive but should be a big chunk of the callsites that are missing `%LO` / `%LA`.
Test Plan:
These are tricky to test exhaustively, but I made an attempt to hit most of them:
- Browsed Almanac interfaces.
- Created/browsed Calendar events.
- Enabled/disabled/showed the lock log.
- Browsed repositories.
- Loaded Facts UI.
- Poked at Multimeter.
- Used typeahead for users and projects.
- Browsed Phriction.
- Ran various fulltext searches.
Not sure these are reachable:
- All the lint stuff might be dead/unreachable/nonfunctional?
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19814
Summary: Depends on D19789. Ref T13217. Continue updating things to use the new %Q-flavored conversions instead of smushing a bunch of strings together.
Test Plan: Browsed around, far fewer errors. These changes are largely mechanical in nature.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19790
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.
Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hach-que
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19789
Summary:
Ref T13217. This method is slightly tricky:
- We can't safely return a string: return an array instead.
- It no longer makes sense to accept glue. All callers use `', '` as glue anyway, so hard-code that.
Then convert all callsites.
Test Plan: Browsed around, saw fewer "unsafe" errors in error log.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19784
Summary: See D19117. Instead of automatically figuring this out inside `phutil_tag()`, explicitly add rel="noreferrer" at the application level to all external links.
Test Plan:
- Grepped for `_blank`, `isValidRemoteURIForLink`, checked all callsites for user-controlled data.
- Created a link menu item, verified noreferrer in markup.
- Created a link custom field, verified no referrer in markup.
- Verified noreferrer for `{nav href=...}`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19118
Summary:
Depends on D19009. Ref T13053. For "Must Encrypt" mail, we must currently strip the "Thread-Topic" header because it sometimes contains sensitive information about the object.
I don't actually know if this header is useful or anyting uses it. My understanding is that it's an Outlook/Exchange thing, but we also implement "Thread-Index" which I think is what Outlook/Exchange actually look at. This header may have done something before we implemented "Thread-Index", or maybe never done anything. Or maybe older versions of Excel/Outlook did something with it and newer versions don't, or do less. So it's possible that an even better fix here would be to simply remove this, but I wasn't able to convince myself of that after Googling for 10 minutes and I don't think it's worth hours of installing Exchange/Outlook to figure out. Instead, I'm just trying to simplify our handling of this header for now, and maybe some day we'll learn more about Exchange/Outlook and can remove it.
In a number of cases we already use the object monogram or PHID as a "Thread-Topic" without users ever complaining, so I think that if this header is useful it probably isn't shown to users, or isn't shown very often (e.g., only in a specific "conversation" sub-view?). Just use the object PHID (which should be unique and stable) as a thread-topic, everywhere, automatically.
Then allow this header through for "Must Encrypt" mail.
Test Plan: Processed some local mail, saw object PHIDs for "Thread-Topic" headers.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19012
Summary:
Depends on D18924. Ref T13048. Each adapter defines which repetition options ("every time", "only the first time") users may select for rules.
Currently, this is all explicit and hard-coded. However, every adapter really just implements this rule (except for some bugs, see below):
> You can pick "only the first time" if this adapter fires more than once on the same object.
Since we already have a `isSingleEventAdapter()` method which lets us tell if an adapter fires more than once, just write this rule in the base class and delete all the copy/pasting.
This also fixes two bugs because of the copy/pasting: Pholio Mocks and Phriction Documents did not allow you to write "only the first time" rules. There's no reason for this, they just didn't copy/paste enough methods when they were implemented.
This will make a future diff (which introduces an "if the rule did not match last time" policy) cleaner.
Test Plan:
- Checked several different types of rules, saw appropriate options in the dropdown (pre-commit: no options; tasks: first or every).
- Checked mocks and wiki docs, saw that you can now write "only the first time" rules.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18925
Summary: Events with no attendees (e.g. fresh instances of recurring events) would trigger an exception when sending notifications, because `$attendee_map` would still get populated.
Test Plan: Declined event, restarted daemons. Did not see exception. Accepted event, restarted daemons. Saw "[Calendar] [Reminder]" email.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18835
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary: Ref T12819. Adds ferret engine support for Calendar events.
Test Plan: Indexed and queried calendar events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18568
Summary: Fixes T12821. (Some events only happen once, so the default behavior doesn't let you pick this rule, and objects must opt into it by saying "yeah, I support multiple edits".)
Test Plan:
Note "only the first time" selected:
{F4999093}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12821
Differential Revision: https://secure.phabricator.com/D18117
Summary: Grep for phui-tag-shade and verify we're no longer calling shade-color directly.
Test Plan: Search, workboard, story points, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17993
Summary:
Fixes T12661.
When changing the start date of an event from some time in the past to some time significantly in the future (more than 24 hours), we'd invalidate only future caches and leave users in an "away" state. Instead, just invalidate all past and future caches (this is simpler than trying to figure out a narrower window, and should not make us do too much extra work).
When uninviting users from events, their caches also didn't get cleared correctly. Instead, clear them.
Test Plan:
- Changed an event from "Apr 1 - June 1" to "May 15 - June 1", saw availablity clear properly.
- Uninvited user `@dog` from an ongoing event, saw availability clear properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12661
Differential Revision: https://secure.phabricator.com/D17809
Summary:
Fixes T12356.
- In this mail, we currently render "6:00 AM". Instead, render "6:00 AM (PDT)" or similar. This is consistent with times in other modern Transaction mail.
- Previously, we would render "UTC-7". Render "PDT" instead. For obscure zones with no known timezone abbreviation, fall back to "UTC-7".
Test Plan:
- Used `bin/calendar notify --minutes X` to trigger notifications, read email bodies.
- Used this script to list all `T` values and checked them for sanity:
```lang=php
<?php
$now = new DateTime();
$locales = DateTimeZone::listIdentifiers();
foreach ($locales as $locale) {
$zone = new DateTimeZone($locale);
$now->setTimeZone($zone);
printf(
"%s (%s)\n",
$locale,
$now->format('T'));
}
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12356
Differential Revision: https://secure.phabricator.com/D17646
Summary:
Ref T11816. Depends on D17644. When you executed a query like "upcoming, limit 5 events" you might match some recurring events starting from, say, a year ago and repeating every month.
We'd then generate the first 5 ghosts for these events (say, last January, February, ... May) and later throw them out, so the correct events in the query window (say, this April) would never get generated.
Instead, generate ghosts beginning with the start of the window. The fix in D17644 to number results correctly allows us to do this.
Test Plan:
- Made a query panel showing 5 events, scheduled an event long in the past, did not visit any of the instances of it so they didn't generate concrete objects.
- Before the patch, near-future instances failed to show; after the patch, they show.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17645
Summary:
Ref T11816. Two minor issues:
- We used `$event`, not `$next_event`, as the event providing the PHID for "Busy at <event name>". This rendered "Busy at <most future event>" on the profile instead of "Busy at <next upcoming event".
- The TTL computation used the event start, not the event end, so we could end up rebuilding the cache too often for users busy at an event.
Test Plan:
- Attended an event in the near future and one later on.
- Saw profile now say "busy at <near future event>" correctly.
- In DarkConsole "Services" tab, no longer saw unnecessary cache refills while attending an event.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17643
Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.
Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.
I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.
Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17631
Summary: Ref T10390. Simplifies dropdown by rolling out canUseInPanel in useless panels
Test Plan: Add a query panel, see less options.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17341
Summary:
See D16676. When an export has an unsupported mode (bad database value, out-of-date object, etc) the intent of this code is to put it into the `<select />` so that you can save the form without silently changing the object.
However, it incorrectly calls `array_shift()` instead of `array_unshift()`.
Test Plan:
Edited a Calendar export with an invalid mode, saw the mode appear properly in the dropdown:
{F2957321}
Reviewers: vrana, chad
Reviewed By: vrana
Differential Revision: https://secure.phabricator.com/D17369
Summary: Ref T11816. The underlying format of recurrence end dates swapped around a bit and we now try to compare `null` to a valid date if you're setting it for the first time.
Test Plan:
- On a new event, set a recurrence end date.
- Then, removed a recurrence end date.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D17150
Summary: Ref T11801. In some cases, this could lead to us failing to generate the first recurrence in a series.
Test Plan: Imported `weekly.ics` (from D16974) and saw an event correctly occur on Aug 18, with my local timezone set to "America/Los_Angeles".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11801
Differential Revision: https://secure.phabricator.com/D16975
Summary:
Fixes T11909. Ref T11816. Instead of offering a dropdown with choices between "Edit/Cancel/Reinstate This Event" and "Edit/Cancel/Reinstate Future Events", make the choice more explicit.
This dialog ends up pretty wordy but this edit is rare, so I think that's alright.
Test Plan: {F2046863}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816, T11909
Differential Revision: https://secure.phabricator.com/D16956
Summary:
Ref T11816. Currently, if someone in California creates an event and then someone in New York edits it, we generate a no-op "<user> changed the start time from 3PM to 3PM." transaction.
This is because the internal timezone of the event is changing, but the actual absolute time is not.
Instead, when an edit wouldn't reschedule an event and would only change the internal timezone, ignore the edit.
Test Plan:
- Edited non-all-day events in PST / EST with out making changes (ignored).
- Edited non-all-day events in PST / EST with changes (changes worked).
- Performed the same edits with all-day events, which also were ignored and worked, respectively.
- Pulled events in and out of all-day mode in different timezones, behavior seemeed reasonable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16955
Summary:
Ref T11816. I don't really know what happened here, maybe I rewrote and broke this at the last second?
In most cases, we directly respect the `isAllDay` flag on the event, so the internal date state doesn't matter too much.
However, in the case of mail notifications, the raw internal state is relevant. This should fix mail notifications for all-day events.
(I might still turn them off since I'm not sure they're too useful, but it's good to have them working.)
Test Plan:
- Created a new all-day event, verified database values wrote correctly.
- Ran `bin/calendar notify --trace`, verified it picked up an all-day event tomorrow with a large enough `--minutes` value.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16954
Summary: Ref T11816. We currently color circles green if you're invited, but should color them green if you're attending, too.
Test Plan: Viewed calendar mobile month view, saw attending events in green.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16899
Summary:
Ref T11816. In some cases, Calendar would only export a subset of events because the "export" flag was ignored or the "display" parameter applied an improper date range to the query.
- Make sure the `export` flag gets processed, even though it isn't a "real" field on the search engine.
- Clear the "display" parameter to avoid date range windowing coming from the day/month logic.
Test Plan: Exported a "display=month" view, verified future events came with it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16898
Summary: Ref T11816. This could be a little cleaner, but we currently have two copies of the logic. Get them using the same code. Once that's actually working I can go make the code a little prettier.
Test Plan: Viewed Calendar month view tooltips, saw the same values as subheaders.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16880
Summary:
Ref T11816.
- Host gets weird behavior around defaulting to the viewer.
- Invitees get weird behavior around defaulting to the viewer.
- "All Day" is just sort of weird since start / end date aren't customizable.
- Recurring/Frequency are weird here and don't make much sense.
I can't immediately come up with reasons that any of these are particularly useful/valuable to default. More of them can be made editable after T10222 gets sorted out.
Test Plan: Edited edit engine custom forms for Calendar events, saw a more sensible list of customizable fields (e.g., policy stuff).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16878
Summary: Uh, non-all-day-events said 1:30 - 2:29 PM, which is real silly.
Test Plan: Looked at a non-all-day-event.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16874
Summary:
Fixes T11809. Ref
- Explicitly document the summary icon hints -- I don't think these are too hard to figure out (and maybe this stuff should just go in the tooltips) but we can start here.
- Use color + shape to distinguish between "cancelled" and "declined", not just color (for users with vision accessibility issues).
- Translate a "minute(s)" string into sensible English.
- Use RSVP status on the month view green circle thing.
Test Plan:
- Read docs.
- Looked at month view.
- Read reminder mail.
- Viewed month view mobile view.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16872
Summary:
Ref T11816. Since the dashboard got updated, Differential now interprets "Responsible Users: epriestley" to mean "epriestley, or any project or package epriestley is part of". You can query for just "epriestley" with "exact(epriestley)".
Give Calendar invites the same behavior: "epriestley" means "any event epriestley is invited to, or a project they are a member of is invited to". Individual invites can be queried with "exact(epriestley)".
This is a little bit copy-pastey but I want to wait for a third use case to clean it up since I think I'm going to have to do a bunch of generalization around "how does an individual PHID get turned into a bunch of PHIDs".
Test Plan: Queried for "Invited: dog", "invited: viewer", "invited; exact(dog)", etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16870
Summary:
Ref T11816. Projects can be invited to an event, but the UI is currently fairly agnostic about them.
Instead, introduce the idea of "RSVPs", which are basically invites for you as an individual or for any group you're a part of. When we go to check if you're invited, we check for you individually first, then check for any groups you belong to if you haven't already accepted/declined.
On the calendar detail page:
- Show the quick "Join" / "Decline" buttons if any project you're a member of is invited.
- If you're invited, highlight any projects which you're a member of to make that more clear.
On other calendar views:
- If you're invited as part of a project, show the "multiple users" icon.
- If it's just you, continue showing the "add one user" icon.
Test Plan: Viewed month view, day view, detail view. Invited groups and individuals. Invited "Dog Project", accepted invite as user "Dog".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16868
Summary:
Ref T11816. This logic was correct, we just did all the work and then mostly threw away the results. This worked correctly anyway in some timezones.
Instead, actually use `$min_date` and `$max_date`.
Test Plan: In "America/Toronto" with server in a more-western timezone, viewed a "Nov 11 - Nov 12" all-day event, saw those dates.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16867
Summary: Ref T11816. This got dropped somewhere along the way, so the mobile month view no longer showed a green-colored hint if a day has events you're invited to.
Test Plan: Viewed Calendar month view on mobile, saw green circles for days with invited events.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11816
Differential Revision: https://secure.phabricator.com/D16852
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
Summary:
Fixes T11836. See some prior discussion in T8376#120613.
The policy hint in headers in the UI is not exhaustive, and can not reasonably be exhaustive. For example, on a revision, it may say "All Users", but really mean "All users who can see the space this object is in and the repository it belongs to, plus the revision author and reviewers".
These rules are explained if you click (and, often, in the documentation), but "All Users" is still at least somewhat misleading.
I don't think there's any perfect solution here that balances the needs of both new and experienced users perfectly, but this change tries to do a bit better about avoiding cases where we say something very open (like "All Users") when the real policy is not very open.
Specifically, I've made these changes to the header:
- Spaces are now listed in the tag, so it will say `(S3 > All Users)` instead of `(All Users)`. They're already listed in the header, this just makes it more explicit that Spaces are a policy container and part of the view policy.
- Extended policy objects are now listed in the tag, so it will say `(S3 > rARC > All Users)` for a revision in the Arcanist repository which is also in Space 3.
- Objects can now provide a "Policy Codex", which is an object that represents a rulebook of more sophisticated policy descriptions. This codex can replace the tag with something else.
- Imported calendar events now say "Uses Import Policy" instead of, e.g., "All Users".
I've made these changes to the policy dialog:
- Split it into more visually separate sections.
- Added an explicit section for extended policies ("You must also have access to these other objects: ...").
- Broken the object policy rules into a "Special Rules" section (for rules like "you can only see a revision if you can see the repository it is part of") and an "Object Policy" section (for the actual object policy).
- Tried to make it a little more readable?
- The new policy dialogs are great to curl up with in front of a fire with a nice cup of cocoa.
I've made these changes to infrastructure:
- Implementing `PhabricatorPolicyInterface` no longer requires you to implement `describeAutomaticCapability()`.
- Instead, implement `PhabricatorPolicyCodexInterface` and return a `PhabricatorPolicyCodex` object.
- This "codex" is a policy rulebook which can set all the policy icons, labels, colors, rules, etc., to properly explain complex policies.
- Broadly, the old method was usually either not useful (most objects have no special rules) or not powerful enough (objects with special rules often need to do more in order to explain them).
Test Plan:
{F1912860}
{F1912861}
{F1912862}
{F1912863}
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T11836
Differential Revision: https://secure.phabricator.com/D16830
Summary:
Ref T11809. I missed this when adding a "Busy" status.
Also the other dot is orange? Just make them all orange for consistency.
Test Plan: Viewed `@username` of busy users (orange), away users (red).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11809
Differential Revision: https://secure.phabricator.com/D16819
Summary: Ref T7643. When we send mail about a change to a package description, allow it to say "CHANGES TO PACKAGE DESCRIPTION" instead of "EDIT DETAILS". Smooth!
Test Plan: {F1909417}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7643
Differential Revision: https://secure.phabricator.com/D16818
Summary:
Ref T11801. These are pretty fiddly because users expect to see the end time for timed events ("10 AM - 11 AM" is ONE hour long) but not for all-day events ("Nov 2 - Nov 3" is TWO days long!)
We also want to store the thing the user actually entered so we don't lose data if they un-all-day the event later.
This may take a little more fiddling since it feels a little shaky, but I couldn't break this version immediately.
Test Plan: Imported a French holiday, got proper display in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11801
Differential Revision: https://secure.phabricator.com/D16815
