Skip to content

ENH Add custom confirmation page#1384

Open
BrandSven wants to merge 1 commit intosilverstripe:7from
brandcom:7
Open

ENH Add custom confirmation page#1384
BrandSven wants to merge 1 commit intosilverstripe:7from
brandcom:7

Conversation

@BrandSven
Copy link

Description

Add a new option to the Userform configuration: Custom confirmation page.

If this field contains a valid PageID from the SiteTree, when a form is successfully submitted in the frontend, the browser is redirected to the given page.

This allows form confirmation pages to be built individually, using all available content blocks (on an elemental page), allowing more flexibility/variety.

Manual testing steps

  • Create or edit a userform
  • Open the configuration tab
  • Select any page in the new field "custom confirmation page"
  • Fill out and submit the form in the frontend

If there's no validation errors, you will be redirected to the selected confirmation page.

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • [i] The commit messages follow our commit message guidelines
    Note: Commit message is missing the ENH prefix. This was discovered after the change was already pushed. Changing the commit message (using rebase) will probably do more harm than good, so we decided against it. Please check if this is a show stopper for this PR
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@michalkleiner
Copy link
Contributor

I think normally we don't add translations in a PR, they are managed via Transifex through a separate process (I believe). Should those be removed from the PR, @GuySartorelli?

@GuySartorelli
Copy link
Member

Correct, thanks @michalkleiner.
@BrandSven Please remove all of the translation additions from this PR. You can submit those via Transifex as per https://docs.silverstripe.org/en/6/contributing/translations/

@GuySartorelli
Copy link
Member

Also you can add the ENH prefix to the commit with git commit --ammend and then git push --force-with-lease to override the commit in the PR.
Though if you choose not to do so I can (hopefully) remember to fix it by squash merging.

Comment on lines +560 to +562
if ($this->data()->ConfirmationPageID) {
$confirmationPage = $this->data()->ConfirmationPage();
if ($confirmationPage && $confirmationPage->exists()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($this->data()->ConfirmationPageID) {
$confirmationPage = $this->data()->ConfirmationPage();
if ($confirmationPage && $confirmationPage->exists()) {
$confirmationPage = $this->data()->ConfirmationPage();
if ($confirmationPage && $confirmationPage->exists()) {

This is a simpler way to do the condition - though not by any means necessary for merging.

Comment on lines +122 to +125
/**
* @var array
*/
private static $has_one = [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* @var array
*/
private static $has_one = [
private static array $has_one = [

New API should be strongly typed instead of relying on PHPDocs.

Comment on lines +222 to +227
),
TreeDropdownField::create(
'ConfirmationPageID',
_t('SilverStripe\\UserForms\\Model\\UserDefinedForm.CONFIRMATIONPAGE', 'Custom confirmation page'),
SiteTree::class
),
Copy link
Member

Choose a reason for hiding this comment

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

This formfield should already be scaffolded as per SiteTree::scaffoldFormFieldForHasOne(). Is there a need to redeclare it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing so allows the use of a new translation key.
There might be other ways of achieveing this that the author is unaware of.

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