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.

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

Feature: Add campaign setting for default form #7614

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

Conversation

kjohnson
Copy link
Member

@kjohnson kjohnson commented Nov 7, 2024

Resolves GIVE-1493

Description

This PR adds the Default Form setting in the Campaign Details Page, on the Settings tab.

Affects

  • Adds CampaignDetailsData DTO class
  • Updates response data to use DTO in CampaignRequestController

Visuals

image

Testing Instructions

Go to the Campaign Details Page
Go to the Settings tab
Select a default form
Publish the campaign
Check that the default form was updated on the Default Form widget

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 kjohnson marked this pull request as ready for review November 8, 2024 18:17
…hub.com:impress-org/give into feature/campaign-default-form-setting-GIVE-1493
Copy link
Contributor

@glaubersilva glaubersilva left a comment

Choose a reason for hiding this comment

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

@kjohnson I found a few problems while testing it and recorded a video to demonstrate that, here is the link: https://www.loom.com/share/f77ce3b6d30b4932ac81edb8d308972f?sid=48028945-a72f-469b-9eaa-1e67190af3e7

@@ -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

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.

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.

'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

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.


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.

Copy link
Member

@alaca alaca left a comment

Choose a reason for hiding this comment

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

Hey Kyle, nice job man.
What would be really cool is that we use the useEntityRecord hook to get the campaign data, we don't have to localize scripts then. The defaultFormId should be a property of the campaign entity so we don't have to use the react-hook-forms watch function to get the form id because the entity will reflect that once it's updated.

'id' => $form->id,
'title' => $form->hasProperty('settings') ? $form->settings->formTitle : $form->title,
];
}, $campaign->forms()->getAll()),
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants