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

Forms contact tab #5685

Closed
wants to merge 17 commits into from
Closed

Forms contact tab #5685

wants to merge 17 commits into from

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Feb 6, 2018

Q A
Bug fix?
New feature? y
Automated tests included?
Related user documentation PR URL mautic/documentation#265
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks?
Deprecations?

Bugfix/feature donate by www.newcom4u.ch

Description:

PR added forms as tab to contacts detail page.
Also PR add new option to forms - "Show results in contacts tab"
This feature allow users add relevant forms contact result to details (for example - orders, reservation etc)

image

image

Steps to test this PR:

  1. Apply PR and run DB migration php app/console doctrine:migrations:migrate or php app/console doctrine:schema:update --force
  2. Create form and enable option "Show results in contacts tab"
  3. Send form
  4. Go to contacts detail page and see if new tab Forms is showed with relevant data
  5. Test case If contact don't have any forms fill
  6. Test case If Mautic user don't have permission don't see forms

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Feb 6, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Feb 6, 2018

Label: Feature request

@mautibot mautibot added the feature A new feature for inclusion in minor or major releases label Feb 6, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Feb 6, 2018

Label: WIP

@mautibot mautibot added the WIP PR's that are not ready for review and are currently in progress label Feb 6, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Feb 8, 2018

Label: WIP

@mautibot mautibot removed the WIP PR's that are not ready for review and are currently in progress label Feb 8, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Feb 8, 2018

Label: Ready to test

@mautibot mautibot added the ready-to-test PR's that are ready to test label Feb 8, 2018
@mautibot mautibot removed the code-review-needed PR's that require a code review before merging label Feb 9, 2018
Copy link

@tpilet tpilet left a comment

Choose a reason for hiding this comment

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

I test everything is ok

@tpilet
Copy link

tpilet commented Feb 9, 2018

Tested. Works properly +1“

@dbhurley dbhurley added this to the 2.13.0 milestone Mar 4, 2018
@escopecz escopecz self-assigned this Mar 22, 2018
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

I noticed a few issues. Please look at them. Also I'm worried that this doesn't have pagination. It has hard-coded limit of 999 rows. It can take some time to load all those data considering all the data that the contact detail page is already loading.

$builder->createField('inContactTab', 'boolean')
->columnName('in_contact_tab')
->nullable()
->build();
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

/**
* @return mixed
*/
public function getInContactTab()
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Does it really return mixed? The $inContactTab property is annotated as bool. (the same in the setter)

If it will be bool, it would be nice to call this method isInContactTab() as it makes it more human friendly :)


/**
* Return Forms with results to contacts tab
*ľ.
Copy link
Sponsor Member

@escopecz escopecz Mar 22, 2018

Choose a reason for hiding this comment

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

What is this Slovak weirdo doing here? 🤣 (and bellow too)

* Return Forms with results to contacts tab
*ľ.
*
* @param null $leadId
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This type hint should have been int|null, right?

'RETURN_ARRAY'
);
if ($permissions['form:forms:viewown'] || $permissions['form:forms:viewother']) {
if (!$permissions['form:forms:viewother']) {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Is the first if necessary 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.

It's from here. I am gonna fixed it

$permissions = $security->isGranted(['form:forms:viewown', 'form:forms:viewother'], 'RETURN_ARRAY');

@mautibot mautibot added the pending-test-confirmation PR's that require one test before they can be merged label Mar 22, 2018
@escopecz escopecz added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Mar 22, 2018
@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged needs-documentation PR's that need documentation before they can be merged and removed code-review-needed PR's that require a code review before merging pending-feedback PR's and issues that are awaiting feedback from the author labels Mar 26, 2018
@Maxell92 Maxell92 self-assigned this Mar 26, 2018
@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Mar 26, 2018
@Maxell92 Maxell92 added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged pending-feedback PR's and issues that are awaiting feedback from the author and removed code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged labels Mar 26, 2018
@Maxell92
Copy link
Contributor

This PR works, great job @kuzmany.

Can you add a documentation? We can merge it then.

@kuzmany
Copy link
Member Author

kuzmany commented Mar 26, 2018

@Maxell92 thanks, docs added mautic/documentation#265

Label: Pending feedback

@mautibot mautibot removed the pending-feedback PR's and issues that are awaiting feedback from the author label Mar 26, 2018
@Maxell92
Copy link
Contributor

Great, thank you. We will merge it after all issues in that PR will be solved :)

@alanhartless alanhartless added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Mar 26, 2018
@alanhartless
Copy link
Contributor

alanhartless commented Mar 26, 2018

Questions on this. What is the use of this one when this exact same data is displayed in the History tab? If you click the button on the left of the timeline entry for the form results, it displays the form result data.

@alanhartless alanhartless removed this from the 2.13.0 milestone Mar 26, 2018
@dbhurley
Copy link
Member

This is not something that belongs in core as a new feature. Alan has correctly identified the reason behind this.

@dbhurley dbhurley closed this Mar 27, 2018
@dbhurley
Copy link
Member

My suggestion would be to create a plugin for this behavior if it's still desired.

@kuzmany
Copy link
Member Author

kuzmany commented Mar 31, 2018

Why you don't vote to community if they interest to the feature? Feature is useful, with better UX and you selective forms, instead timeline views. This PR added little touch of CRM, you can attach to profile Orders, Contacts, Request a demo etc. In future we can expend this PR with many other features.

Maybe you can create channel when you get feedback before PR is done (ready to commit with the documentation). We should save a lot of time.

Hope @tpilet or @npracht appreciate that.

@alanhartless
Copy link
Contributor

So how is this different than what is already shown in the History tab for form submission entries though? This at face value is a duplicate feature for what’s already there.

Also keep in mind that this is a marketing automation project, not a CRM. It’s not in interest of the project to make it a CRM.

@kuzmany
Copy link
Member Author

kuzmany commented Mar 31, 2018

You can choose relevant forms. Many forms we use, just few are relevant to contact for company business. Maybe you can ask non-tech people which variant do they like. I bet, If they want work with certain contact, this PR is more helpful like timeline entries.

@alanhartless
Copy link
Contributor

Yes but there are lots of relevant parts of Mautic and forms are just part of it. Some may put more value on emails opened or assets downloaded or campaigns over forms submitted. At some point, displaying the same data in multiple places will become bloatware.

@tpilet
Copy link

tpilet commented Mar 31, 2018 via email

@alanhartless
Copy link
Contributor

I can understand the value in that. But that’s your specific marketing strategy. I’ve seen a lot of marketing done through Mautic that does not rely on forms as different marketers put an emphasis on different parts of Mautic. This would only benefit a subset of the marketers leveraging your same strategy.

That’s why we feel this would be better suited as a plug-in since it’s replicating the information already displayed in the same page but adds emphasize to the data for a specific marketing strategy.

@kuzmany
Copy link
Member Author

kuzmany commented May 15, 2018

Finally bring this feature as plugin https://github.com/kuzmany/mautic-extendee-form-tab-bundle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature for inclusion in minor or major releases needs-documentation PR's that need documentation before they can be merged pending-feedback PR's and issues that are awaiting feedback from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants