Summary: Depends on D20668. Ref T13343. Just an easy cleanup/simplification while I'm here.
Test Plan: `grep` for `getActionConstant()`
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20669
Summary:
Fixes T13327. Currently, when we try to bill an account and all members are disabled, we fail temporarily and the task retries forever.
At least for now, just treat this as a permanent failure.
Test Plan:
- Used `bin/phortune invoice` to generate a normal invoice for a regular subscription.
- Disabled all the account members, then tried again. Got a helpful permanent failure:
```
$ ./bin/phortune invoice --subscription PHID-PSUB-kbedwt5cyepoc6tohjq5 --auto-range
Set current time to Mon, Jun 24, 2:47 PM.
Preparing to invoice subscription "localb.phacility.com" from Fri, May 31, 10:14 AM to Sun, Jun 30, 10:14 AM.
WARNING
Manually invoicing will double bill payment accounts if the range overlaps an
existing or future invoice. This script is intended for testing and
development, and should not be part of routine billing operations. If you
continue, you may incorrectly overcharge customers.
Really invoice this subscription? [y/N] y
[2019-06-24 14:47:57] EXCEPTION: (PhabricatorWorkerPermanentFailureException) All members of the account ("PHID-ACNT-qp54y3unedoaxgkkjpj4") for this subscription ("PHID-PSUB-kbedwt5cyepoc6tohjq5") are disabled. at [<phabricator>/src/applications/phortune/worker/PhortuneSubscriptionWorker.php:88]
arcanist(head=experimental, ref.master=d92fa96366c0, ref.experimental=db4cd55d4673), corgi(head=master, ref.master=6371578c9d32), instances(head=stable, ref.master=ba9e4a19df1c, ref.stable=37fb1f4917c7), libcore(), phabricator(head=master, ref.master=65bc481c91de, custom=11), phutil(head=master, ref.master=7adfe4e4f4a3), services(head=master, ref.master=5424383159ac)
#0 PhortuneSubscriptionWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:124]
#1 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:163]
#2 PhabricatorWorker::scheduleTask(string, array, array) called at [<phabricator>/src/applications/phortune/management/PhabricatorPhortuneManagementInvoiceWorkflow.php:169]
#3 PhabricatorPhortuneManagementInvoiceWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:457]
#4 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:349]
#5 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/setup/manage_phortune.php:21]
$
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13327
Differential Revision: https://secure.phabricator.com/D20613
Summary: Ref PHI1166. I'm documenting our dependencies, and we have approximately 5,000 lines of external code to support WePay as a Phortune provider. We don't use it, I'm almost certain it doesn't work, and we have no plans to use it in the near future. If we did pursue it, I'd probably just wrap the API in a 100-line `WePayFuture` anyway since 5K lines of dependencies to make a couple method calls is ridiculous.
Test Plan: Grepped for `wepay`, `httpful`, `restful`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aurelijus
Differential Revision: https://secure.phabricator.com/D20521
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: Ref T13250. See D20149. In a number of cases, we use `setQueryParams()` immediately after URI construction. To simplify this slightly, let the constructor take parameters, similar to `HTTPSFuture`.
Test Plan: See inlines.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20151
Summary: Ref T13249. See D20132. Although we're probably a poor way to validate a big list of stolen cards in practice in production today (it's very hard to quickly generate a large number of small charges), putting rate limiting on "Add Payment Method" is generally reasonable, can't really hurt anything (no legitimate user will ever hit this limit), and might frustrate attackers in the future if it becomes easier to generate ad-hoc charges (for example, if we run a deal on support pacts and reduce their cost from $1,000 to $1).
Test Plan: Reduced limit to 4 / hour, tried to add a card several times, got rate limited.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20158
Summary: Ref T13244. See PHI1052. Our error handling for Stripe errors isn't great right now. We can give users a bit more information, and a less jarring UI.
Test Plan:
Before (this is in developer mode, production doesn't get a stack trace):
{F6197394}
After:
{F6197397}
- Tried all the invalid test codes listed here: https://stripe.com/docs/testing#cards
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20132
Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.
Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.
This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.
Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.
Test Plan:
- Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
- Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20121
Summary:
See PHI1023. Ref T7607. Occasionally, companies need their billing address (or some other custom text) to appear on invoices to satisfy process or compliance requirements.
Allow accounts to have a custom "Billing Name" and a custom "Billing Address" which appear on invoices.
Test Plan: {F6134707}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T7607
Differential Revision: https://secure.phabricator.com/D19979
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 T13217. This older query does some manual joins; update it for more modern joins.
Test Plan: Ran `instances/` unit tests and got a clean result, browsed Phortune merchants.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19820
Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".
There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.
Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19801
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:
See PHI689. It can be difficult to distinguish between cards with the same number but different expiration dates (common when the bank sends you a new card).
For now, show the expiration date on the cart checkout screen.
Test Plan: Viewed a cart checkout screen with multiple cards, saw expiration dates.
Reviewers: amckinley
Differential Revision: https://secure.phabricator.com/D19462
Summary:
Ref T4340. Some "Register/Login" and "Link External Account" buttons are forms which submit to third-party sites. Whitelist these targets when pages render an OAuth form.
Safari, at least, also prevents a redirect to a third-party domain after a form submission to the local domain, so when we first redirect locally (as with Twitter and other OAuth1 providers) we need to authorize an additional URI.
Test Plan: Clicked all my registration buttons locally without hitting CSP issues.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19159
Summary:
See PHI399. Ref T4340. This header provides an additional layer of protection against various attacks, including XSS attacks which embed inline `<script ...>` or `onhover="..."` content into the document.
**style-src**: The "unsafe-inline" directive affects both `style="..."` and `<style>`. We use a lot of `style="..."`, some very legitimately, so we can't realistically get away from this any time soon. We only use one `<style>` (for monospaced font preferences) but can't disable `<style>` without disabling `style="..."`.
**img-src**: We use "data:" URIs to inline small images into CSS, and there's a significant performance benefit from doing this. There doesn't seem to be a way to allow "data" URIs in CSS without allowing them in the document itself.
**script-src** and **frame-src**: For a small number of flows (Recaptcha, Stripe) we embed external javascript, some of which embeds child elements (or additional resources) into the document. We now whitelist these narrowly on the respective pages.
This won't work with Quicksand, so I've blacklisted it for now.
**connect-src**: We need to include `'self'` for AJAX to work, and any websocket URIs.
**Clickjacking**: We now have three layers of protection:
- X-Frame-Options: works in older browsers.
- `frame-ancestors 'none'`: does the same thing.
- Explicit framebust in JX.Stratcom after initialization: works in ancient IE.
We could probably drop the explicit framebust but it wasn't difficult to retain.
**script tags**: We previously used an inline `<script>` tag to start Javelin. I've moved this to `<data data-javelin-init ...>` tags, which seems to work properly.
**`__DEV__`**: We previously used an inline `<script>` tag to set the `__DEV__` mode flag. I tried using the "initialization" tags for this, but they fire too late. I moved it to `<html data-developer-mode="1">`, which seems OK everywhere.
**CSP Scope**: Only the CSP header on the original request appears to matter -- you can't refine the scope by emitting headers on CSS/JS. To reduce confusion, I disabled the headers on those response types. More headers could be disabled, although we're likely already deep in the land of diminishing returns.
**Initialization**: The initialization sequence has changed slightly. Previously, we waited for the <script> in bottom of the document to evaluate. Now, we go fishing for tags when domcontentready fires.
Test Plan:
- Browsed around in Firefox, Safari and Chrome looking for console warnings. Interacted with various Javascript behaviors. Enabled Quicksand.
- Disabled all the framebusting, launched a clickjacking attack, verified that each layer of protection is individually effective.
- Verified that the XHProf iframe in Darkconsole and the PHPAST frame layout work properly.
- Enabled notifications, verified no complaints about connecting to Aphlict.
- Hit `__DEV__` mode warnings based on the new data attribute.
- Tried to do sketchy stuff with `data:` URIs and SVGs. This works but doesn't seem to be able to do anything dangerous.
- Went through the Stripe and Recaptcha workflows.
- Dumped and examined the CSP headers with `curl`, etc.
- Added a raw <script> tag to a page (as though I'd found an XSS attack), verified it was no longer executed.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19143
Summary:
See support email. There's nothing tricky here, we were just missing a check. The different parts of this got built at different times so I think this was simply overlooked.
Also add a redundant check just to future-proof this and be on the safe side.
Test Plan: Used `bin/phortune invoice` to charge a pact subscription. After deleting the card, the charge failed with an appropriate error.
Reviewers: amckinley
Differential Revision: https://secure.phabricator.com/D19020
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: 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: Fixes T12958. Adds a success message when card is added, also switches to use radio buttons for clarity. Updated redirect uri for deleting methods as well.
Test Plan:
Add cards, remove cards.
{F5091084}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12958
Differential Revision: https://secure.phabricator.com/D18381
Summary:
Ref T12681. We need this to update the "paid until" window on support pacts.
(Instance billing doesn't use this because everything just checks if you have unpaid invoices, nothing actually happens when you pay them.)
Test Plan: See D18187.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12681
Differential Revision: https://secure.phabricator.com/D18188
Summary: Ran across a few straglers. Convert to the correct color.
Test Plan: grep for profile-image-button, check profile image selection page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18096
Summary: Try to dis-ambiguate various button types and colors. Moves `simple` to `phui-button-simple` and moves colors to `button-color`.
Test Plan: Grep for buttons still inline, UIExamples, PHUIX, Herald, and Email Preferences.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18077
Summary: This says "Edit Subscription" but the page only lets you update the payment method for autopay. I think this is confusing users. I tried to find the best language here, but suggest something else if you prefer.
Test Plan: Review language on sample subscription page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17944
Summary: Ref T12685. Checks merchant capabilities at the edit engine level
Test Plan: Test with and without admin level permissions. Get restricted access if I cheat. Still able to create with admin.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17852
Summary:
Ref T12451. This is a GREAT comment (A++) but we only need one copy of it.
This uses a pattern similar to Projects, which is a little weird but works well enough.
Test Plan:
- Viewed all four tabs of an account.
- Viewed a page with a bad account ID which 404'd properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17694
Summary: Ref T12451. This code is the same as the other code.
Test Plan: Went through the default-account case with this code, worked the same as the other code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17693
Summary:
Ref T12451. Ref T12484. This should deal with all the `+` / `-` / `=` cases correctly, I think.
Also makes sure that members are real users, not commits or tokens or whatever. And expands the creation test case to make some other basic sanity checks.
Test Plan:
- Went through implicit first-time creation flow.
- Went through explicit second-time creation flow.
- Unit test now passes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12484, T12451
Differential Revision: https://secure.phabricator.com/D17692
Summary:
Ref T12451. Ref T12484. I think D17657 fixed this, but caused the bug in D17690. The fix for that causes this bug again.
Put a unit test on it. This test currently fails; I'll correct the bug in the next change.
Test Plan: Ran `arc unit`, saw a failure.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12484, T12451
Differential Revision: https://secure.phabricator.com/D17691
Summary:
Ref T12451. When you explicitly created a second or third account or whatever, you wouldn't be added as a member.
(The editor sees that you're "already a member", so it doesn't add you.)
Test Plan:
- Go to `/phortune/`.
- Click "Switch Accounts".
- Click "Create Account".
- Create an account.
- Before patch: unable to view it since you don't get added as a member.
- After patch: account created with you as member.
- Also created an accont with multiple members.
- Tried to create an account with no members.
- Tried to create an account with just someone else as a member.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17690
Summary: Ref T12451. `$this->getAccount()` may not return an account.
Test Plan:
- Visit `/phortune/X/`, where `X` is the ID of an account you don't have permission to view.
- Before patch: fatal.
- After patch: normal policy exception page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12451
Differential Revision: https://secure.phabricator.com/D17689
Summary: Primarily, this splits individual sections of the single account page into a more managable and robust sidenav for subscriptions, billing, and managers. The functionality on the subpages is light, but I expect to build on then in coming diffs. This also starts building out a more effective "status" area on the lead page.
Test Plan:
- Load up default account
- Make some edits
- Click on each of the new navigation items
- Verify links to "see all" work
- Test overdue and no payment states for status
{F4337317}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17589
Summary: There is currently a validation error triggered if you initialize a new account without a member set. I think this is the correct fix, but let me know.
Test Plan: truncate phortune_account database, navigate to phortune, see account automatically created to "Default Account".
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17657
Summary: Builds out Phortune Merchant pages to have a sidenav and sub-pages for further expansion. For now this links Orders and Subscriptions to the query engine pages, but could be split out to be more informative (unpaid, upcoming, etc).
Test Plan:
Create a new merchant, edit some information, add a manager in new UI, edit logo, click through to subscriptions, orders.
{F4883013}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17655
Summary: This updates the backend of PhortuneAccount to use EditEngine and Modular Transactions and updates language to "account manager" for clarity of role.
Test Plan:
- Wiped `phortune_account` table
- Visit Phortune, see new account automatically created.
- Edit name and managers
- Try to set no name or remove myself as a manager, get error messages
- Visit `/phortune/` and create another new account
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17585
Summary: Modernize PhortuneMerchant for Modular Transactions. Also changed the language of "Members" to "Managers", which I think fits better given the power/capability.
Test Plan:
- Create a new Merchant
- Test not filling in a name, see error
- Test removing myself, see error
- Edit an existing Merchant
- Add new managers
- Test removing myself, see error
- Replace Picture
- Update various fields, contact info, email, footer
- Verify transactions are now nice and pretty
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17584
Summary: Minor, uses 'user-circle' for account, and merchant logo for merchants in lists.
Test Plan: View the landing page, see updated logos and icons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17586
Summary: Move individual controller files into cooresponding folders. Makes it easier to locate sections and expand without clutter. Also made "chargelist" part of account since it's tied to having an account specifically.
Test Plan: Vist charges, merchants, subscription, accounts, and other pages. No errors from file move.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17587
Summary: Fixes T12224. This brings "Autopay" on the View controller into line with how it works on the Edit controller.
Test Plan:
- Viewed subscriptions with no autopay, valid autopay, and deleted autopay.
{F2750725}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12224
Differential Revision: https://secure.phabricator.com/D17334
Summary:
Currently, when a payment method is invalid we still render the full name and let you save the form without making changes. This can be confusing.
Instead:
- Render "<Deleted Payment Method>", literally.
- Render an error immediately.
- Prevent the form from being saved without changing the method.
Test Plan: {F1955487}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16935
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:
When Phortune merchant accounts are created via mechanisms other than the web UI (for example, by Phacility unit tests) this validation check may fail.
Transactions are validated even if no transactions of the given type are being applied, to allow the editor to raise errors like "Name is required!".
If there's no TYPE_INVOICEEMAIL transaction, we'll get called with empty `$xactions` and fail on `strlen($new_email)` because the variable is never defined.
As a secondary issue, if contactInfo, invoiceEmail or invoiceFooter are not provided the record will fail to insert (none of these are nullable).
Test Plan: Ran Phacility unit tests, got a clean result for new instance creation.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16798
Summary: Makes a more complete PDF looking invoice form for printing in Phortune.
Test Plan: Make an invoice, click print view, print.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16762
Summary:
This fixes the permissions issue with D16750, which is actually not really a permissions issue, exactly.
This is the only place anywhere that we use a tokenizer field //and// give it a default value which is not the same as the object value (when creating a merchant, we default it to the viewer).
In other cases (like Maniphest) we avoid this because you can edit the form to have defaults, which would collide with whatever default we provide. Some disucssion in T10222.
Since we aren't going to let you edit these forms for the forseeable future, this behavior is reasonable here though.
However, it triggered a sort-of-bug related to conflict detection for these fields (see T4768). These fields actually have two values: a hidden "initial" value, and a visible edited value.
When you submit the form, we compute your edit by comparing the edited value to the initial value, then applying adds/removes, instead of just saying "set value equal to new value". This prevents issues when two people edit at the same time and both make changes to the field.
In this case, the initial value was being set to the display value, so the field would say "Value: [(alincoln x)]" but internally have that as the intitial value, too. When you submitted, it would see "you didn't change anything", and thus not add any members.
So the viewer wouldn't actually be added as a member, then the policy check would correctly fail.
Note that there are still some policy issues here (you can remove yourself from a Merchant and lock yourself out) but they fall into the realm of stuff discussed in D16677.
Test Plan: Created a merchant account with D16750 applied.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16764
Summary: Converts PhortuneMerchant to EditEngine.
Test Plan: Edits existing merchants fine, same issue as Conpherence when making new ones with permissions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16750
Summary: Is a logo. For merchants.
Test Plan: Set a new logo, remove it. See on list.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7607
Differential Revision: https://secure.phabricator.com/D16751
Summary:
A live instance hit the scenario described in the comment, where an out-of-date user was being selected as the actor.
Since they were no longer an account member, they could not see the payment method and autopay was failing.
Instead, select a relatively arbitrary user who is a current, valid, non-disabled member.
Test Plan: Ran subscriptions with `bin/worker execute ...`, saw it select a valid actor.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16757
Summary: Part of making this look/feel/be more professional is having decent receipts for billing, including contact information (whatever we want to put in there). I'm not using this anywhere at the moment, but will.
Test Plan: Add Contact Info, see Contact Info. Also, why is Remarkup not rendering with line breaks? Seems to be a OneOff thing... anywho... bears!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7607
Differential Revision: https://secure.phabricator.com/D14125
