1up-lab / contao-mailchimp

Contao bundle for MailChimp subscribe/unsubscribe forms (using MailChimp API V3.0)
MIT License
7 stars 3 forks source link

Enhancement: Field sorting #6

Closed bastibuck closed 7 years ago

bastibuck commented 7 years ago

Cheers,

really enjoy your extension, thanks! I'm using it in a Contao 3.5 and it works good.

But there is one small thing I would like to see: if I add a field in MailChimp before the email field it doesn't show up in front of the email field in my website.

MailChimp: image

Website: image

This is due to the code in ModuleSucscribe.php where you add the email field (line 59) before you go over each field (line 70) that is additionally added in MailChimp. Would be awesome if you could change the code so that it looks for the order in MailChimp and outputs it to the website as well.

If you need more info let me know.

Thanks, bastibuck

bastibuck commented 7 years ago

PS: For now I'm overwriting your compile() in a custom Class and reorder fields this way. If I come up with a general solution I will create a PR, but unfortunately it's not a high priority right now since it works now.

bytehead commented 7 years ago

Nice catch. I'll try to implement/fix this.

bezin commented 7 years ago

@bastibuck how do you even manage to add a form field before the email field? I just tried to investigate this issue and I even fail to reproduce this 8-)

bastibuck commented 7 years ago

Hehe ;-)

You can not change order in Settings-->List fields and |MERGE| tags but you can in Signup forms --> General forms --> Signup form

The order you pick here (drag'n'drop) will also be visible in the |MERGE| tags view.

Hope this helps

bezin commented 7 years ago

Indeed – that is weird, though :D Anyway, I gonna look into this.

bytehead commented 7 years ago

In the very beginning there was only the email field. It's probably because of that. I'll check this and bring up a PR if we can change it.

bezin commented 7 years ago

There might be another issue: the api call to "lists/$listid/merge-fields" only returns all the other fields, but not the email field. It also does not reflect the order of the fields, even though the order changes in the list fields view of the MailChimp interface…

bytehead commented 7 years ago

the api call to "lists/$listid/merge-fields" only returns all the other fields, but not the email field.

Ah, I remember this.

bytehead commented 7 years ago

So this will basically be a known limitation?

bezin commented 7 years ago

Not necessarily🍾 I just found out, that the response includes the display order for every merge field (http://developer.mailchimp.com/documentation/mailchimp/reference/lists/merge-fields/#read-get_lists_list_id_merge_fields).

The email field is not included though, but if we have e.g. three fields (first name, last name, email) and the following display order, we may guess its position:

1 for first_name and 2 for last_name, email must be the last. 2 for first_name and 3 for last_name, email must be the first. 1 for first_name and 3 for last_name, email must be second.

bastibuck commented 7 years ago

As long as display order always gives a consecutive order it should be possible to find the email field's position with your solution easily. Like the idea!

we may guess its position

It's not even guessing, it's more like finding - absolutely fine with finding a position ;)

bezin commented 7 years ago

I'll try it again with some more fields, to check if it is "always" in consecutive order. With the given orders above that was the case.

bezin commented 7 years ago

I tried it with up to seven fields – it works as expected. The display_order field is always in a consecutive order, the place of the email field left out. I am gonna fork it and create a PR for this :)

bytehead commented 7 years ago

Thank you in advance! 👍

bastibuck commented 7 years ago

Thanks :-)

bytehead commented 7 years ago

I'll close this in favor of #7, thanks to you guys!

bezin commented 7 years ago

Thanks for the quick merge. I was just gonna say we could make this backwards compatible, but anyway, it is a small extension, the upgrade should be quite easy ;)

bytehead commented 7 years ago

You're welcome. Yeah, we could have make this BC, but it's not worth the effort anyway 😄

bytehead commented 7 years ago

3.0.0 contains a nasty bug, subscribe won't work. Update to 3.0.1.