pentacent / keila

Open Source Newsletter Tool.

https://keila.io

GNU Affero General Public License v3.0

1.26k stars 61 forks source link

Improved Contact Activity Stream + Statistics Page + Error Handling #173

Closed wmnnd closed 1 year ago

wmnnd commented 1 year ago

This PR implements #168 and is a complete refactoring of the way recipient events (e.g. unsubscribes, bounces, clicks, etc) are handled. It also includes an improved display of that information in the contact and campaign stats views. activity-stream

wmnnd commented 1 year ago

The new contact activity stream data is now also used on a new and improved campaign stats page: keila-improved-stats

wmnnd commented 1 year ago

@panoramix360: If you want to give this a try, I think it’s ready to merge now :-)

panoramix360 commented 1 year ago

Taking a look right now! I notice that you updated the Elixir to 1.14, I was going to work on it this week.

But it's great that you already managed to do it!

Congratulations on reaching this UX/UI design, I'm too bad in UX/UI to reach this state haha I'm only good at coding it haha

panoramix360 commented 1 year ago

I analyzed two points for now:

Update Elixir version on Dev Container

We need to update the docker-compose.yml and Dockerfile inside the dev_container directory with the new version of Elixir, since we updated it on the project.

I already updated it locally for testing, and it worked.

Chart.js

I saw that you used the chart.js library to create the graphics, really nice addition and it'll be good to use it on future features.

But the way it is done with the chart hook currently, could be challenging to maintain or modularize, maybe a good idea would be to create more JavaScript files to break the code. I tend to think that for the front end, there are some things that the Phoenix framework doesn't help us too much hehe

I notice some console.log as well on the insertOrUpdateGraph function, it's good to remove them.

Tomorrow morning, I'll resume my review and let you know.

I hope that what I already reviewed could help the project :smile:

wmnnd commented 1 year ago

@panoramix360 Thanks for giving it a look! :heart:

I’ve now updated the Elixir version for the devcontainer setup to 1.14.0 as well.

Also good that you noticed the console.log :sweat_smile: I agree that the current JS code isn’t very modular; we should probably refactor it at some point when we need more graphs in other places.

wmnnd commented 1 year ago

I’ve added some code that improves error handling in the delivery queue. This should help with campaigns being stuck on "sending" in case of delivery errors. Fixes #142.

panoramix360 commented 1 year ago

Great! I'll continue the review then!

wmnnd commented 1 year ago

Now also implements #172.

wmnnd commented 1 year ago

@panoramix360 & @gbottari: Do you have more notes on this PR or do you think we’re good to go? :relaxed:

panoramix360 commented 1 year ago

Hi @wmnnd

Sorry for not answering earlier, I had a full week.

I just want to make some assertions to see if I understood correctly:

contacts.ex

On the method update_contact_status, if there is an error getting the contact, I think the error handling is not being dealt with.
It's great that you refactored a little the way the project_id is passed down to the contact to be created, it's simpler.

builder.ex

I didn't understand the literal ID used on line 39 to create the Recipient

mailings.ex

In the method recipient_stats, the hardcoded queries are not that great, but I don't know if there is a more robust way to do it hehe

In general, the other updates were pretty straightforward, so congratulations :D

I think it's good to merge, we can do some improvements later as well when we test some more with users.

Sorry again for not being more present on this, I need to organize my schedule better to work on this repo more haha

wmnnd commented 1 year ago

Thanks for your notes! I have added an explicit nil return for the update_contact_status function. Maybe the functions should actually throw an exception instead, but at least it’s explicit now :smile:

The literal ID in builder.ex is just a valid UUID for when there is no actual recipient - e.g. when creating an email preview.

I also agree that the queries for the campaign stats could probably be optimized, but they seem to be working quite well so far :)

panoramix360 commented 1 year ago

Great! :D

Two issues closed with one PR.

When I mentioned the queries I was referring to the way they are written on strings, not about the performance hehe, but yeah, we can try to fix this later