Notification #214

herdigiorgi · 2018-01-29T18:28:00Z

No description provided.

facundoolano · 2018-01-30T01:22:31Z

src/reminders/remind_router.erl

@@ -24,6 +25,12 @@ send_test(User, Channel, HolidayDate) ->
  {ok, Pid} = supervisor:start_child(remind_router_sup, []),
  gen_server:cast(Pid, {send_reminder, User, Channel, HolidayDate, Message, true}).

+send_message(User, Channel, Message) ->


if you introduce this generic send_message function then send_test should not exist anymore and use the new one to test the reminder

its not possible because send_test accepts HolidayDate, and send_message sets the date internally. Maybe we need something more general like notification_handler, with a function send(User, Channel, MessageType, Message). Where MessageType can be a reminder, test, notification, or something else in the future.

Yeah but what I mean is you should have one send_message function that takes all the parameters. Then you can have a higher level function like send_reminder that builds the reminder specficic message and then callssend_message. send_test could be removed, I think it's enough to just call the generic send_message with the test specific message.

facundoolano · 2018-01-30T01:25:03Z

src/handlers/channel_item_handler.erl

@@ -58,7 +58,8 @@ from_json(Req, State = #{is_new := true,
     } when is_list(DaysBefore), length(DaysBefore) =< 5 ->
      CleanConfig = maps:filter(fun not_empty_value/2, Config),
      {ok, _} = db_channel:create(Email, Name, Type, CleanConfig, DaysBefore, Time, TimeZone),
-
+      {ok, Channel} = db_channel:get(Email, Name),


db_channel:create already returns the channel. You should understand the previous code before introducing a new line.

facundoolano · 2018-01-30T01:25:43Z

src/handlers/channel_item_handler.erl

@@ -99,6 +100,8 @@ from_json(Req, State = #{is_new := false,
  end.

 delete_resource(Req, State = #{email := Email, name := Name}) ->
+  {ok, Channel} = db_channel:get(Email, Name),


this isn't necessary, the channel is already part of the State (check the rest of the code).

facundoolano · 2018-01-30T01:26:57Z

src/handlers/channel_item_handler.erl

+  remind_router:send_message(User, Channel, Msg).
+
+send_creation_notification(Channel=#{name := Name}, #{user := User}) ->
+  Msg = <<"Crated a remainder '", Name/binary, "'.">>,


-Crated
+Created

herdigiorgi · 2018-01-30T15:49:44Z

I can't figure out why the test of reminders_SUITE.erl on line 257 fails:

{ok, Channel} = db_channel:get(Email, Name),

test output error:

%%% reminders_SUITE ==> {{badmatch,{ok,200,
               [{<<"server">>,<<"Cowboy">>},
                {<<"date">>,<<"Tue, 30 Jan 2018 15:46:01 GMT">>},
                {<<"content-length">>,<<"2">>},
                {<<"content-type">>,<<"application/json">>}],
               []}},
 [{reminders_SUITE,monthly_limit_enforced,1,
                   [{file,"holiday_ping/_build/test/lib/holiday_ping/test/reminders_SUITE.erl"},
                    {line,257}]},
  {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1529}]},
  {test_server,run_test_case_eval1,6,[{file,"test_server.erl"},{line,1045}]},
  {test_server,run_test_case_eval,9,[{file,"test_server.erl"},{line,977}]}]}

facundoolano · 2018-01-30T17:48:27Z

test/reminders_SUITE.erl

  Message = <<"This is a holiday reminder: John Doe will be out today.">>,
+  Reminders = ets_channel:get_reminders(TableId, Email),
+  true =lists:any(
+          fun({ReminderEmail, ReminderMessage}) ->


it's more readable to assign the fun to a variable and the pass that to lists:any

facundoolano · 2018-01-30T17:51:41Z

src/reminders/remind_router.erl

+  UserName = maps:get(name, User),
+  FinalMessage = <<UserName/binary, ": ", Message/binary>>,
+  {ok, Pid} = supervisor:start_child(remind_router_sup, []),
+  gen_server:cast(Pid, {send_reminder, User, Channel, erlang:date(), FinalMessage, true}).


another issue is that right now, the log of sent reminders in the database has a boolean column test. Now, apart from regular reminders and test reminders, we introduced these notifications. We should probably remove the test column and have a reminder type or something, to properly inform in the log.

Yes, i believe that creating a "message type" or something like that would be the most flexible way of doing it.

herdigiorgi · 2018-01-31T21:41:38Z

Should I try to make remind_router more general, and make a message_router, capable of handle the 3 types of message we currently have? For that i will need to change the db schema.

facundoolano · 2018-02-02T23:38:14Z

I think you should change the db schema to replace the test boolean for a message type field as discussed. You should also add the send_message function, but don't think you need to rename remind_router or change it's interface other than that (at least don't do it yet, let's see how the code looks after these changes and decide if it needs more refactoring).

facundoolano · 2018-02-02T23:40:14Z

test/channels_api_SUITE.erl

+                                           Body),
+  ok = ktn_task:wait_for_success(
+         fun() ->
+             [{Email, _}] = ets_channel:get_reminders(TableId, Email),


this has to check that the message text is the one expected

facundoolano · 2018-02-02T23:40:23Z

test/channels_api_SUITE.erl

+                                           "/api/channels/test_ets"),
+  ok = ktn_task:wait_for_success(
+         fun() ->
+             [{Email, _}, {Email, _}] =


facundoolano · 2018-02-02T23:43:26Z

test/channel_test_SUITE.erl

+  Message = <<"This is a Holiday Ping test: John Doe will be out on holidays.">>,
+  Reminders = ets_channel:get_reminders(TableId, Email),
+  true = lists:any(
+           fun({ReminderEmail,ReminderMessage}) ->


you could do

fun({Email, Message}) -> true; (_OtherEmail, _OtherMessage) -> false end

facundoolano · 2018-02-02T23:45:24Z

test/channels_api_SUITE.erl

@@ -102,3 +103,37 @@ delete_channel(Config) ->
    {ok, 204, _, _} = test_utils:api_request(delete, Token, "/api/channels/my_delete_channel"),
    {ok, 404, _, _} = test_utils:api_request(delete, Token, "/api/channels/my_delete_channel"),
    ok.
+
+create_delete_channel_notification(Config) ->


you also need to test the edit part: if you add a new target to the channel, the notification should go only to the new one; if you remove one, only the removed should receive the notification.

unbalancedparentheses · 2018-02-11T16:34:10Z

@hernanex3 add a better title to the PR

herdigiorgi added 4 commits January 29, 2018 14:59

delete notification

392a0a0

notification of creation & deletetion of channels

f857942

sending notification tests

96ec212

better ets name for test

5d6d4df

facundoolano reviewed Jan 30, 2018

View reviewed changes

herdigiorgi added 2 commits January 30, 2018 12:35

two test fixed

72cd3a8

refactoring

78a0cf9

facundoolano reviewed Jan 30, 2018

View reviewed changes

test check for 2 notifications now

5322f91

facundoolano reviewed Feb 2, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification #214

Notification #214

herdigiorgi commented Jan 29, 2018

facundoolano Jan 30, 2018

herdigiorgi Jan 31, 2018 •

edited

Loading

facundoolano Feb 2, 2018

facundoolano Jan 30, 2018

facundoolano Jan 30, 2018

facundoolano Jan 30, 2018

herdigiorgi commented Jan 30, 2018 •

edited

Loading

facundoolano Jan 30, 2018

facundoolano Jan 30, 2018

herdigiorgi Jan 31, 2018

herdigiorgi commented Jan 31, 2018

facundoolano commented Feb 2, 2018

facundoolano Feb 2, 2018

facundoolano Feb 2, 2018

facundoolano Feb 2, 2018

facundoolano Feb 2, 2018

unbalancedparentheses commented Feb 11, 2018

Notification #214

Are you sure you want to change the base?

Notification #214

Conversation

herdigiorgi commented Jan 29, 2018

Choose a reason for hiding this comment

herdigiorgi Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herdigiorgi commented Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herdigiorgi commented Jan 31, 2018

facundoolano commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unbalancedparentheses commented Feb 11, 2018

herdigiorgi Jan 31, 2018 •

edited

Loading

herdigiorgi commented Jan 30, 2018 •

edited

Loading