pentacent / keila

Open Source Newsletter Tool.
https://keila.io
GNU Affero General Public License v3.0
1.36k stars 67 forks source link

Add campaign custom data #104

Closed wmnnd closed 2 years ago

wmnnd commented 2 years ago

Implements #103

Here is campaign data in action: Screenshot 2022-01-14 at 17-19-51 Open Source Newsletter Tool · Keila

This PR does the following:

wmnnd commented 2 years ago

@panoramix360, @gbottari: I’d really appreciate it if you could give this PR a look and let me know what you think! :pray:

wmnnd commented 2 years ago

@panoramix360 Excellent! I’ve added some more details of what the PR changes in the PR description.

wmnnd commented 2 years ago

I think there are some bugs in the editor in this branch, but I’m already working on fixing those.

panoramix360 commented 2 years ago

HI @wmnnd, I looked into your PR, sorry for the delay, the PR is very long hehe

Pretty nice feature! I think that will be good to have this on Keila! Congrats!

Some suggestions:

I tested and found some minor bugs:

I noticed something strange too, I'll try to write the steps so you can reproduce:

Maybe don't letting a campaign being created if the user has not configured any senders will be a better choice, I don't know.

Sorry for the extense reply haha

Tomorow, me and @gbottari will begin looking into the rate limiter issue.

But if you want I can help you tackle this issues above!

Let me know what you think :)

gbottari commented 2 years ago

Hi guys! I think @panoramix360 made some great points. I also think this PR is great because it will enable the personalization of newsletters.

I noticed that JsonField does not allow JSON arrays. But it's not clear to me why that is. I think it is a valid use case. Imagine one wishes to reference a contact recommended products, like ['apples', 'oranges', 'bananas'].

I also think it is worth limiting the maximum size of contact.data and campaign.data to prevent abuse (maybe adding a constraint).

wmnnd commented 2 years ago

Hey @panoramix360 and @gbottari, thanks for this very useful feedback!

So here are the things that still need fixing as a takeaway from your comments:

panoramix360 commented 2 years ago

I'm glad that you liked our feedback, if you need any more help with this PR, just let us know

We'll now focus our efforts in the send rate issue

wmnnd commented 2 years ago

I’m working on a new modal for adding Liquid tags, this should make things a bit easier! What do you think?

Screenshot 2022-02-10 at 22-26-19 Open Source Newsletter Tool · Keila

panoramix360 commented 2 years ago

I think this is a great opportunity to show the user what the feature can do!

wmnnd commented 2 years ago

The Liquid dialog is now ready! Let me know what you think, I think this PR is ready to merge :partying_face:

panoramix360 commented 2 years ago

I tested and now I think it's pretty nice, well done

It'll be a great addition to the project :)

wmnnd commented 2 years ago

@panoramix360 Thanks for testing it and all the feedback you’ve given, that’s been really helpful!