Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature: Add campaign setting for default form #7614

Open
wants to merge 7 commits into
base: epic/campaigns
Choose a base branch
from
Open

Feature: Add campaign setting for default form #7614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb51c53
feature: Add campaign setting for default form
kjohnson Nov 7, 2024
592e74f
Merge branch 'epic/campaigns' into feature/campaign-default-form-sett…
kjohnson Nov 8, 2024
3f0e276
refactor: Update default form state
kjohnson Nov 8, 2024
2732910
Merge branch 'feature/campaign-default-form-setting-GIVE-1493' of git…
kjohnson Nov 8, 2024
5cddc0a
Merge branch 'epic/campaigns' into feature/campaign-default-form-sett…
kjohnson Nov 8, 2024
0bea8ff
Merge branch 'feature/campaign-default-form-setting-GIVE-1493' of git…
kjohnson Nov 8, 2024
f50c964
wip
kjohnson Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/Campaigns/Actions/LoadCampaignDetailsAssets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Give\Campaigns\Actions;

use Give\Campaigns\Models\Campaign;
use Give\DonationForms\Models\DonationForm;
use Give\Framework\Support\Facades\Scripts\ScriptAsset;
use Give\Helpers\Form\Utils;

Expand Down Expand Up @@ -33,14 +34,17 @@ public function __invoke()
true
);

$defaultForm = $campaign->defaultForm();
$defaultFormTitle = Utils::isV3Form($defaultForm->id) ? $defaultForm->settings->formTitle : $defaultForm->title;
wp_localize_script($handleName, 'GiveCampaignDetails',
[
'adminUrl' => admin_url(),
'currency' => give_get_currency(),
'isRecurringEnabled' => defined('GIVE_RECURRING_VERSION') ? GIVE_RECURRING_VERSION : null,
'defaultForm' => $defaultFormTitle,
'donationForms' => array_map(function(DonationForm $form) {
return [
'id' => $form->id,
'title' => $form->hasProperty('settings') ? $form->settings->formTitle : $form->title,
];
}, $campaign->forms()->getAll()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a way to load this form list beyond the first page load because it can get inaccurate easily depending of how user add new forms to the campaign - check my attached video for more details.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a list of forms available for re-use from the Campaign Forms List Table?

Copy link
Contributor

@glaubersilva glaubersilva Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjohnson I don't know if our List Table component provides some way to access the list of available items through a JS object or something like that, but I think not, even because the list table can have pagination which makes me think again that this field to update the default form trough the settings tab is not a good idea because in situations where we have a massive amount of forms in a single campaign, populate this dropdown can become a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already have a registered campaign entity, we should use the useEntityRecord hook. As I see it, we should just update the CampaignDetailsData DTO to include the forms associated with the campaign.

]
);

Expand Down
11 changes: 8 additions & 3 deletions src/Campaigns/Controllers/CampaignRequestController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@

namespace Give\Campaigns\Controllers;

use Exception;
use Give\Campaigns\Models\Campaign;
use Give\Campaigns\Repositories\CampaignRepository;
use Give\Campaigns\ValueObjects\CampaignGoalType;
use Give\Campaigns\ValueObjects\CampaignRoute;
use Give\Campaigns\ValueObjects\CampaignStatus;
use Give\Campaigns\ValueObjects\CampaignType;
use Give\Campaigns\DataTransferObjects\CampaignDetailsData;
use Exception;
use WP_Error;
use WP_REST_Request;
use WP_REST_Response;
Expand All @@ -31,7 +32,9 @@ public function getCampaign(WP_REST_Request $request)
return new WP_Error('campaign_not_found', __('Campaign not found', 'give'), ['status' => 404]);
}

return new WP_REST_Response($campaign->toArray());
return new WP_REST_Response(
(new CampaignDetailsData($campaign))->toArray()
);
}

/**
Expand Down Expand Up @@ -139,7 +142,9 @@ public function updateCampaign(WP_REST_Request $request)
$campaign->save();
}

return new WP_REST_Response($campaign->toArray());
return new WP_REST_Response(
(new CampaignDetailsData($campaign))->toArray()
);
}


Expand Down
38 changes: 38 additions & 0 deletions src/Campaigns/DataTransferObjects/CampaignDetailsData.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php

namespace Give\Campaigns\DataTransferObjects;

use Give\Campaigns\Models\Campaign;
use Give\Framework\Support\Contracts\Arrayable;

/**
* @unreleased
*/
class CampaignDetailsData implements Arrayable
{
/**
* @var Campaign
*/
private $campaign;

/**
* @unreleased
*/
public function __construct(Campaign $campaign)
{
$this->campaign = $campaign;
}

/**
* @unreleased
*/
public function toArray(): array
{
return array_merge(
$this->campaign->toArray(),
[
'defaultFormId' => $this->campaign->defaultForm()->id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this DTO since the campaign model will have the default form ID available when this PR is merged: #7616

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'm looking at #7616 now.

]
);
}
}
2 changes: 1 addition & 1 deletion src/Campaigns/Routes/RegisterCampaignRoutes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ public function getSchema(): array
'description' => esc_html__('Campaign goal type', 'give'),
],
'defaultFormId' => [
'type' => 'integer',
'type' => 'number',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is any specific reason to use number instead of integer here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that reactHookForms is validating as a number. When I used integer it would throw an error. Likewise, the goal property of the schema uses number. The id property uses integer, but the id isn't being validated with reactHookForms.

Copy link
Contributor

@glaubersilva glaubersilva Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjohnson In this case, I think the best approach is to create a react-hook-form custom rule in the fronted to validate integer numbers because changing this parameter here to number will not reflect what exactly this endpoint accepts as a parameter. If someone using our API uses the OPTIONS to retrieve the parameters schema for this endpoint, they will see that the id can be a number which is wrong since it can only be an integer value. Also, if we decide to provide some documentation for our API using a plugin like the wp-api-swaggerui (or any other similar tool) the docs generated by it will display the wrong type for this parameter as well.

'description' => esc_html__('Default campaign form ID', 'give'),
],
],
Expand Down
1 change: 1 addition & 0 deletions src/Campaigns/resources/admin/components/CampaignDetailsPage/CampaignDetailsPage.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ select[name="campaignId"] {

.sectionDescription {
font-size: 16px;
line-height: 24px;
color: #4b5563;
}

Expand Down
7 changes: 1 addition & 6 deletions src/Campaigns/resources/admin/components/CampaignDetailsPage/Components/CampaignStats.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,9 @@ import { addQueryArgs } from '@wordpress/url';
import HeaderText from './HeaderText';
import HeaderSubText from './HeaderSubText';
import DefaultFormWidget from "./DefaultForm";
import {GiveCampaignDetails} from "@givewp/campaigns/admin/components/CampaignDetailsPage/types";

const campaignId = new URLSearchParams(window.location.search).get('id');

declare const window: {
GiveCampaignDetails: GiveCampaignDetails;
} & Window;

const pluck = (array: any[], property: string) => array.map(element => element[property])

const filterOptions = [
Expand Down Expand Up @@ -65,7 +60,7 @@ const CampaignStats = () => {
<Column flex={1}>
<GoalProgressWidget />

<DefaultFormWidget defaultForm={window.GiveCampaignDetails.defaultForm} />
<DefaultFormWidget />

</Column>
</Row>
Expand Down
19 changes: 16 additions & 3 deletions src/Campaigns/resources/admin/components/CampaignDetailsPage/Components/DefaultForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,25 @@
import {__} from "@wordpress/i18n";
import HeaderText from './HeaderText';
import HeaderSubText from './HeaderSubText';
import {CampaignFormOption} from "@givewp/campaigns/admin/components/CampaignDetailsPage/types";
import {GiveCampaignDetails} from "@givewp/campaigns/admin/components/CampaignDetailsPage/types";
import {useFormContext} from "react-hook-form";

declare const window: {
GiveCampaignDetails: GiveCampaignDetails;
} & Window;

/**
* @unreleased
*/
const DefaultFormWidget = ({defaultForm}: {defaultForm: string}) => {
const DefaultFormWidget = () => {

const {watch} = useFormContext();

const [defaultFormId] = watch(['defaultFormId']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to not use the useWatch hook instead? I typically use the hook for the performance benefits unless I need to watch many values at the root level:

From the docs about watch

This API will trigger re-render at the root of your app or form, consider using a callback or the useWatch api if you are experiencing performance issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would probably work. I had copy/pasted from another part of the codebase, but I'll refactor and try it.


const defaultFormTitle = window.GiveCampaignDetails.donationForms
.find(form => Number(form.id) === Number(defaultFormId))?.title;

return (
<div style={{
flex: 1,
Expand Down Expand Up @@ -46,7 +59,7 @@ const DefaultFormWidget = ({defaultForm}: {defaultForm: string}) => {
padding: '12px 16px',
borderRadius: '4px',
}}>
{defaultForm}
{defaultFormTitle}
</div>
</div>
)
Expand Down
29 changes: 29 additions & 0 deletions src/Campaigns/resources/admin/components/CampaignDetailsPage/Tabs/Settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ declare const window: {
* @unreleased
*/
export default () => {

const {donationForms} = window.GiveCampaignDetails

const {
register,
watch,
Expand Down Expand Up @@ -136,6 +139,32 @@ export default () => {
{errors.goal && <div className={styles.errorMsg}>{`${errors.goal.message}`}</div>}
</div>
</div>

{/* DEFAULT CAMPAIGN FORM SETTINGS */}
{ donationForms.length > 1 && (
<div className={styles.section}>
<div className={styles.leftColumn}>
<div className={styles.sectionTitle}>{__('Default campaign form', 'give')}</div>
<div className={styles.sectionDescription}>
{__('Your campaign page and blocks will collect donations through this form by default. You can change the default form at any time.', 'give')}
</div>
</div>
<div className={styles.rightColumn}>
<div className={styles.sectionField}>
<div
className={styles.sectionSubtitle}>{__('Select your default campaign form', 'give')}</div>

<select {...register('defaultFormId', {valueAsNumber: true})} disabled={isDisabled}>
{donationForms.map(({id, title}) => <option key={id} value={id}>{title}</option>)}
</select>

{errors.defaultForm &&
<div className={styles.errorMsg}>{`${errors.defaultForm.message}`}</div>}
</div>
</div>
</div>
)}

</div>
);
};
Expand Down
3 changes: 2 additions & 1 deletion src/Campaigns/resources/admin/components/CampaignDetailsPage/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ export interface GiveCampaignDetails {
adminUrl: string;
currency: string;
isRecurringEnabled: boolean;
defaultForm: string;
defaultFormId: number;
donationForms: { id: number; title: string; }[];
}

export type CampaignFormOption = {
Expand Down
Loading