Closed panoramix360 closed 2 years ago
Thanks for implementing this important feature, @panoramix360! :pray:
I just have one suggestion for improvement: I think the new fields for the rate limits should be added to SenderConfig
instead of Sender
. Like this, they can be configured globally for shared senders which makes more sense I believe.
Let me know if you have time to look into this in the coming days - otherwise I can also take care of it :smiley:
@wmnnd no problem!
I'll do it this week then :D
Thanks for implementing this important feature, @panoramix360! pray
I just have one suggestion for improvement: I think the new fields for the rate limits should be added to
SenderConfig
instead ofSender
. Like this, they can be configured globally for shared senders which makes more sense I believe.Let me know if you have time to look into this in the coming days - otherwise I can also take care of it smiley
@wmnnd, I was looking into it but now I have some doubts.
Regarding the UI, do you think this needs to be configured per shared sender? Or the fields will be default for all senders?
Currently, they are like this:
So if we change per shared sender, these fields will appear inside each shared sender, is that it?
The limit should be configurable for regular senders that don't use a shared sender - and for shared senders (where it will then be used for all senders that use this shared sender).
So it probably makes sense to add the UI fields after this line: https://github.com/pentacent/keila/blob/main/lib/keila_web/templates/sender/_config.html.heex#L52 Then they will be displayed among the other settings for each sender adapter.
@wmnnd done!
Can you review and check the PR? :)
Awesome, thanks for the update! I’ll review it this weekend.
@panoramix360, @gbottari: Sorry for the delay, I’m on it! :sweat_smile:
Hi @wmnnd! Did you have the chance to review it?
I saw that you added some modifications to keys and configs. Looks good.
@panoramix360, @gbottari: I did some refactoring and testing and I think now everything is working as it should - both with regular senders and shared senders.
The major changes are:
Let me know what you think! I think it’s ready to be merged now :partying_face:
Hi @wmnnd,
Good additions to the PR, thank you!
I have just one small consideration:
ses_webhook_controller_test.exs
, I think that @tag
is missing on the test.Really nice that you created a TODO too, could be nice to make the algorithm better in the future!
That's it :D I really liked your suggestions and please let me know if you need anything more on this PR.
I'll look through the other issues to solve another :) If you have a suggestion, let me know!
Thanks for noticing the missing tag! I think we’re ready to merge now :partying_face:
Implements the rate limit feature mentioned on issue #107
rate_limit_per_hour
,rate_limit_per_minute
,rate_limit_per_second
)