Feature: add new routes for donations and donors

[glaubersilva]

Related to GIVE-1392 and GIVE-1393

Description

This PR implements 4 new REST API endpoints to retrieve Donations and Donors. In the endpoints that return multiple entries, is possible to filter the returned data using custom parameters in the request and also is possible use pagination and sort the results using the page, per_page, sort and direction parameters.

Another thing to consider is that sensitive data will be returned only if the user making the request is the site administrator.

Sensitive data for donations:

$sensitiveData = [
    'donorIp',
    'email',
    'phone',
    'billingAddress',
];

Sensitive data for donors:

$sensitiveData = [
    'userId',
    'email',
    'phone',
    'additionalEmails',
];

The new endpoints to retrieve a single entry:

/give-api/v2/donations/(?P<id>[0-9]+)

/give-api/v2/donors/(?P<id>[0-9]+)

The new endpoints to retrieve multiple entries:

/give-api/v2/donations

/give-api/v2/donors

Important: These endpoints that return multiple entries allow filtering the returned data through the campaignId parameter. It's also possible to use the hideAnonymousDonations or hideAnonymousDonors parameter to exclude from the results the donations/donors that made anonymous donations. Beyond that, on the /give-api/v2/donors endpoint, it is possible to use the onlyWithDonations parameter to retrieve all donors or just the ones that have valid donations completed.

Sample request including anonymous donations in the results:

$request = new WP_REST_Request('GET' 'give-api/v2/donations');

$request->set_query_params(
    [
        'hideAnonymousDonations' => false,   
        //'campaignId' => $campaign1->id, //Uncomment this line to filter by campaign
    ]
);

Sample request including anonymous donors in the results:

$request = new WP_REST_Request('GET' 'give-api/v2/donors');

$request->set_query_params(
    [
        'hideAnonymousDonors' => false,   
        //'campaignId' => $campaign1->id, //Uncomment this line to filter by campaign
    ]
);

Sample request to retrieve the 5 most recent donations:

$request = new WP_REST_Request('GET' 'give-api/v2/donations');

$request->set_query_params(
    [
        'page' => 1,
        'per_page' => 5,
        'sort' => 'createdAt',
        'direction' => 'DESC',
        //'campaignId' => $campaign1->id, //Uncomment this line to filter by campaign
    ]
);

Sample request to retrieve the top 5 donors:

$request = new WP_REST_Request('GET' 'give-api/v2/donors');

$request->set_query_params(
    [
        'page' => 1,
        'per_page' => 5,
        'sort' => 'totalAmountDonated',
        'direction' => 'DESC',
        //'campaignId' => $campaign1->id, // Uncomment this line to filter by campaign
    ]
);

Affects

GiveWP Rest API endpoints available for public use.

Testing Instructions

In your terminal, run the following commands:

composer test -- --filter GetDonationRouteTest

composer test -- --filter GetDonationsRouteTest

composer test -- --filter GetDonorRouteTest

composer test -- --filter GetDonorsRouteTest

Pre-review Checklist

• Acceptance criteria satisfied and marked in related issue
• Relevant @unreleased tags included in DocBlocks
• Includes unit tests
• Reviewed by the designer (if follows a design)
• Self Review of code and UX completed

[kjohnson]

Cool seeing headers and links for pagination! This is often overlooked, but good that we are starting to leverage this feature.

I added some feedback on the queries. In particular, we have the CampaignDonationQuery which we should consider using - or at least update what is here to account for subscriptions and test payments.

[JasonTheAdams]

Out of curiosity, is the give-api namespace pre-existing? The -api suffix part feels redundant since this is part of the REST API path.

[glaubersilva]

@JasonTheAdams Yes, it's a pre-existing thing.

[JasonTheAdams]

I'm seriously considering recommending a give/v3 or something that deviates from the old REST API.

[JasonTheAdams]

Left some thoughts, @glaubersilva! Great work!

[rickalday]

Passed manual QA tests.

[JasonTheAdams]

@glaubersilva Why did this go to QA with requested changes pending? 😅

[glaubersilva]

@JasonTheAdams It didn't go to QA. I think @rickalday got confused because this PR is related to other tasks that were to QA

[JasonTheAdams]

Hahah! Got it. 😆

[rickalday]

@glaubersilva Why did this go to QA with requested changes pending? 😅

My bad. I posted on the wrong PR.

[JasonTheAdams]

@glaubersilva Why did this go to QA with requested changes pending? 😅

My bad. I posted on the wrong PR.

[glaubersilva]

@JasonTheAdams I liked the idea of renaming /give-api/v2/ to /give/v3/ for new routes, aligning with the new approach we introduced in the Campaigns domain. This change would ensure consistency across all implementations using the new standards, which are designed to support entities.

I believe it would be appropriate to apply this replacement everywhere, including the routes implemented in this PR as well as those for Campaigns.

So, I think we can move forward with this change unless the other devs have concerns or objections about it, let's check with them just to make sure we are not missing something here.

[JasonTheAdams]

Sounds great, @glaubersilva! I like the idea of retroactively applying this so long as the endpoints we're applying them to are:

1. Truly RESTful
2. Not in production

[JasonTheAdams]

@glaubersilva Do we have any restrictions around this parameter? If our goal is to protect anonymous donations, then we may want to include some permission parameters around this. Presently, this is a fully public API, and these parameters are discoverable, so it's not hard for someone to flip the switch on this. There are a few contexts, here:

1. Donor wall
2. Admin list tables (donations list and campaign donations)
3. Campaign donations
4. 3rd Party usage

I'm guessing this will result in 3 forms of output:

1. Anonymous donations are included and donor info revealed (admin-side)
2. Anonymous donations are included but donor information is redacted (donor wall)
3. Anonymous donations are prohibited

Now, we may actually be fine with anyone in the world being able to query anonymous donations so long as the donor information is redacted (dropping option 3). But we'll still need a way to grab all information for the admin side with proper authorization.

Note: this is true of both the collection endpoint and single donation resource endpoint.

cc: @jonwaldstein

[jonwaldstein]

@JasonTheAdams yeah, I align with the options 1 & 2 which sounds like we need to update the permissions to consider this param. As long as the unauthenticated request always respects the anonymous option by redacting donor information - there should be nothing to worry about as we are not exposing unintended information to the public.

[JasonTheAdams]

Exactly. It should throw a 403 if the authorization is invalid for a specific parameter. The parameters should default to whatever version requires the least authorization.

[glaubersilva]

We already are blocking access to sensitive data for non-admin users:

$sensitiveData = [
    'donorIp',
    'email',
    'phone',
    'billingAddress',
 ];

If I understand it well, we also should block access to the "anonymous data" when we are dealing with anonymous donations.

So, I refactored the escDonation() method and implemented a new logic there to also remove these properties:

if ($donation->anonymous) {
    $anonymousData = [
        'donorId',
        'honorific',
        'firstName',
        'lastName',
        'company',
    ];

    $sensitiveData = array_merge($sensitiveData, $anonymousData);
}

I considered here "anonymous data" any property from the donation model that can be used to identify a donor, which makes me wonder if we should remove the "comment" property as well. Currently, I kept it on the result, so would like to know your thoughts on it... Should we keep it or not? I'm asking because depending on the content of the comment is possible to identify the donor (I guess).

Also, I wrote more unit tests to check if the access to these "anonymous data" is being blocked by default and allowed for admin users, see: 7def493 and 238fd0a

[glaubersilva]

One more thing to consider here is that right now we are removing the "anonymous data" from the results. I was wondering if we should replace the content instead of removing it.

For example:

$newValues = [
    'donorId' => 'anonymous',
    'honorific' => 'anonymous',
    'firstName' => 'anonymous',
    'lastName' => 'anonymous',
    'company' => 'anonymous',
];

[jonwaldstein]

Thanks @glaubersilva!

I think you're on the right track. I would say donation comment can be public but written by anonymous (donor details retracted)

Regarding preserving result keys, I'm leaning toward just removing results if not authenticated as I would prefer only receiving real values for my results rather than auto-generated results based on state if that make sense lol

[JasonTheAdams]

I'm thinking that it may be useful to turn anonymousDonations into an enum:

• exclude — (default) simply excludes the donations from the collection
• redact — includes anonymous donations and redacts the sensitive information, replacing with "anonymous"
• include — includes anonymous donations with full details (requires authorization)

For include, if authorization fails we simply 403 and call it a day, as the client is trying to do something beyond their pay grade.

And I agree, Jon, that the donation comment is public. It's presented to the donor as something public when they chose to write a comment.

What do you guys think?

[glaubersilva]

@JasonTheAdams Honestly, I have the impression that transforming it on an enum will overcomplicate things without a clear advantage.

I would prefer to keep it simple as it is right now: you only need to inform if want to include the anonymous donation yes or not - if the current user has permission to see the full data, they will receive the full data, if not, they will receive the redact data.

Did I miss some use cases where is necessary for the admin to return redact data?

[JasonTheAdams]

So this is a RESTful design issue, @glaubersilva . You want to think of REST endpoints as similar to a pure function. That is, you get what you expect based on your parameters. Having the shape of the data change based on conditional things such as authorization means that the result is stateful — which is a RESTful no-no. The result should be predictable by the user.

The reason why the enum is a useful approach is that it gives the client explicit control over the shape of the response. The authorization, required for the include value, is explicit but requires authorization to work. So either it returns exactly what the client expects or does a 403 because they're not allowed to do that.

So it's not about having a use case where the admin may want the data redacted or not; it's about creating a system wherein the client can explicitly determine what they want so we don't have to worry about the scenario (i.e. state) they're working from. That said, if I'm logged into a site and viewing the donor wall, it should be redacted even if I'm logged in as an admin — otherwise, the inconsistent behavior becomes confusing. If the client wants this behavior, then it can adjust the query parameter based on its own conditions.

Hope this helps!

[glaubersilva]

@jonwaldstein @JasonTheAdams Here in the GetDonors route, I think we should remove this parameter that allows include anonymous donations (this is applied only when the onlyWithDonations param is set) because if a donor made a donation as anonymous, retrieving the donor should not be possible.

[glaubersilva]

@jonwaldstein @JasonTheAdams I was thinking better about it and I think we need to keep it...

Let's use this sample to retrieve the top 5 donors of a campaign:

$request = new WP_REST_Request('GET' 'givewp/v3/donors');

$request->set_query_params(
    [
        'page' => 1,
        'per_page' => 5,
        'sort' => 'totalAmountDonated',
        'direction' => 'DESC',
        'campaignId' => $campaign->id,
    ]
);

If one of the top donors retrieved made only anonymous donations, we need to retrieve the information related to the totalAmountDonated but without any other data that can be used to identify the donor. So, I think we shouldn't remove this pram, but do something similar to the approach we are doing for donations related to redact data.

In that scenario, I think makes sense to replace the original data with something like this:

$newValues = [
    'donorId' => 'anonymous',
    'honorific' => 'anonymous',
    'firstName' => 'anonymous',
    'lastName' => 'anonymous',
    'company' => 'anonymous',
];

[JasonTheAdams]

@glaubersilva You alluded to this parameter. This weirds me out. Hahah! This would make sense if this was a constituents endpoint, but having a "I want donors that made donations" is weird. It just naturally raises the question: "How is a person a donor if they didn't donate?" 😆

[JasonTheAdams]

Is there a specific use-case for this filter?

[JasonTheAdams]

@glaubersilva @jonwaldstein
Thinking ahead to database changes, I strongly recommend that we update (or create) a DonorQueryBuilder that has declarative methods for things like this. So this craziness becomes:

$query->whereDonorsHaveDonations();

It's cool for the REST APIs to use the query builder, but we want to reduce modifications that are directly coupled to the database structure (such as referencing meta like we are here).

[jonwaldstein]

@glaubersilva there's one more update i'd like to request that I was discussing with @JasonTheAdams recently:

Let's move the actual registration of the rest endpoints into a shared top-level domain. The reason is our REST API is really it's own domain that should live outside of the business logic of our other domains. It also makes it much easier to manage 😄

It looks like we already have an API domain so we could simply add a REST/V3 folder inside and have the structure look something like this:

We do also have an existing Service Provider that registers our v2 REST endpoints. I'm not opposed of doing something similar within our new API\REST\V3 domain as it pretty clearly registers each route programatically.