Merge pull request #3228 from esl/inbox/fix_system_messages

Inbox-muclight: fix system messages

big_tests/tests/inbox_SUITE.erl

@@ -59,6 +59,7 @@
          leave_and_store_conversation/1,
          groupchat_markers_one_reset_room_created/1,
          groupchat_markers_all_reset_room_created/1,
+         system_message_is_correctly_avoided/1,
          no_aff_stored_and_remove_on_kicked/1,
          no_stored_and_remain_after_kicked/1,
          simple_groupchat_stored_in_all_inbox_muc/1,
@@ -111,81 +112,81 @@ tests() ->
     ].
 
 groups() ->
-    G = [
-         {generic, [parallel],
-          [
-           disco_service,
-           returns_valid_form,
-           returns_error_when_first_bad_form_field_encountered,
-           returns_error_when_bad_form_field_start_sent,
-           returns_error_when_bad_form_field_end_sent,
-           returns_error_when_bad_form_field_order_sent,
-           returns_error_when_bad_form_field_hidden_read_sent,
-           returns_error_when_bad_reset_field_jid,
-           returns_error_when_no_reset_field_jid,
-           returns_error_when_unknown_field_sent
-          ]},
-         {one_to_one, [parallel],
-          [
-           user_has_empty_inbox,
-           msg_sent_stored_in_inbox,
-           msg_with_no_store_is_not_stored_in_inbox,
-           msg_with_store_hint_is_always_stored,
-           carbons_are_not_stored,
-           user_has_two_conversations,
-           msg_sent_to_offline_user,
-           msg_sent_to_not_existing_user,
-           user_has_two_unread_messages,
-           other_resources_do_not_interfere,
-           reset_unread_counter_with_reset_chat_marker,
-           reset_unread_counter_with_reset_stanza,
-           try_to_reset_unread_counter_with_bad_marker,
-           non_reset_marker_should_not_affect_inbox,
-           user_has_only_unread_messages_or_only_read,
-           reset_unread_counter_and_show_only_unread,
-           check_total_unread_count_and_active_conv_count,
-           check_total_unread_count_when_there_are_no_active_conversations,
-           total_unread_count_and_active_convs_are_zero_at_no_activity
-          ]},
-         {muclight, [parallel],
-          [
-           simple_groupchat_stored_in_all_inbox,
-           advanced_groupchat_stored_in_all_inbox,
-           groupchat_markers_one_reset,
-           non_reset_marker_should_not_affect_muclight_inbox,
-           groupchat_reset_stanza_resets_inbox,
-           create_groupchat,
-           leave_and_remove_conversation,
-           groupchat_markers_one_reset_room_created,
-           groupchat_markers_all_reset_room_created
-          ]},
-         {muclight_config, [sequence],
-          [
-           create_groupchat_no_affiliation_stored,
-           leave_and_store_conversation,
-           no_aff_stored_and_remove_on_kicked,
-           no_stored_and_remain_after_kicked
-          ]},
-         {muc, [parallel],
-          [
-           simple_groupchat_stored_in_all_inbox_muc,
-           simple_groupchat_stored_in_offline_users_inbox_muc,
-           unread_count_is_the_same_after_going_online_again,
-           unread_count_is_reset_after_sending_chatmarker,
-           non_reset_marker_should_not_affect_muc_inbox,
-           unread_count_is_reset_after_sending_reset_stanza,
-           private_messages_are_handled_as_one2one
-          ]},
-         {timestamps, [parallel],
-          [
-           timestamp_is_updated_on_new_message,
-           order_by_timestamp_ascending,
-           get_by_timestamp_range,
-           get_with_start_timestamp,
-           get_with_end_timestamp
-          ]}
-        ],
-    ct_helper:repeat_all_until_all_ok(G).
+    [
+     {generic, [parallel],
+      [
+       disco_service,
+       returns_valid_form,
+       returns_error_when_first_bad_form_field_encountered,
+       returns_error_when_bad_form_field_start_sent,
+       returns_error_when_bad_form_field_end_sent,
+       returns_error_when_bad_form_field_order_sent,
+       returns_error_when_bad_form_field_hidden_read_sent,
+       returns_error_when_bad_reset_field_jid,
+       returns_error_when_no_reset_field_jid,
+       returns_error_when_unknown_field_sent
+      ]},
+     {one_to_one, [parallel],
+      [
+       user_has_empty_inbox,
+       msg_sent_stored_in_inbox,
+       msg_with_no_store_is_not_stored_in_inbox,
+       msg_with_store_hint_is_always_stored,
+       carbons_are_not_stored,
+       user_has_two_conversations,
+       msg_sent_to_offline_user,
+       msg_sent_to_not_existing_user,
+       user_has_two_unread_messages,
+       other_resources_do_not_interfere,
+       reset_unread_counter_with_reset_chat_marker,
+       reset_unread_counter_with_reset_stanza,
+       try_to_reset_unread_counter_with_bad_marker,
+       non_reset_marker_should_not_affect_inbox,
+       user_has_only_unread_messages_or_only_read,
+       reset_unread_counter_and_show_only_unread,
+       check_total_unread_count_and_active_conv_count,
+       check_total_unread_count_when_there_are_no_active_conversations,
+       total_unread_count_and_active_convs_are_zero_at_no_activity
+      ]},
+     {muclight, [parallel],
+      [
+       simple_groupchat_stored_in_all_inbox,
+       advanced_groupchat_stored_in_all_inbox,
+       groupchat_markers_one_reset,
+       non_reset_marker_should_not_affect_muclight_inbox,
+       groupchat_reset_stanza_resets_inbox,
+       create_groupchat,
+       leave_and_remove_conversation,
+       groupchat_markers_one_reset_room_created,
+       groupchat_markers_all_reset_room_created
+      ]},
+     {muclight_config, [sequence],
+      [
+       create_groupchat_no_affiliation_stored,
+       leave_and_store_conversation,
+       no_aff_stored_and_remove_on_kicked,
+       no_stored_and_remain_after_kicked,
+       system_message_is_correctly_avoided
+      ]},
+     {muc, [parallel],
+      [
+       simple_groupchat_stored_in_all_inbox_muc,
+       simple_groupchat_stored_in_offline_users_inbox_muc,
+       unread_count_is_the_same_after_going_online_again,
+       unread_count_is_reset_after_sending_chatmarker,
+       non_reset_marker_should_not_affect_muc_inbox,
+       unread_count_is_reset_after_sending_reset_stanza,
+       private_messages_are_handled_as_one2one
+      ]},
+     {timestamps, [parallel],
+      [
+       timestamp_is_updated_on_new_message,
+       order_by_timestamp_ascending,
+       get_by_timestamp_range,
+       get_with_start_timestamp,
+       get_with_end_timestamp
+      ]}
+    ].
 
 suite() ->
     escalus:suite().
@@ -195,14 +196,24 @@ suite() ->
 %%--------------------------------------------------------------------
 
 init_per_suite(Config) ->
-    ok = dynamic_modules:ensure_modules(domain_helper:host_type(mim), inbox_modules()),
+    ok = dynamic_modules:ensure_modules(
+           domain_helper:host_type(mim), inbox_modules()),
+    ok = dynamic_modules:ensure_modules(
+           ct:get_config({hosts, mim, secondary_host_type}),
+           [{mod_inbox,
+             [{aff_changes, false},
+              {remove_on_kicked, true},
+              {groupchat, [muclight]},
+              {markers, [displayed]}]}]),
     InboxOptions = inbox_opts(),
     Config1 = escalus:init_per_suite(Config),
     [{inbox_opts, InboxOptions} | Config1].
 
 end_per_suite(Config) ->
-    HostType = domain_helper:host_type(mim),
-    dynamic_modules:stop(HostType, mod_inbox),
+    dynamic_modules:stop(
+      domain_helper:host_type(mim), mod_inbox),
+    dynamic_modules:stop(
+      ct:get_config({hosts, mim, secondary_host_type}), mod_inbox),
     escalus:end_per_suite(Config).
 
 init_per_group(one_to_one, Config) ->
@@ -214,7 +225,7 @@ init_per_group(muclight, Config) ->
 init_per_group(muclight_config, Config) ->
     ok = dynamic_modules:ensure_modules(domain_helper:host_type(mim), muclight_modules()),
     Config1 = inbox_helper:reload_inbox_option(Config, groupchat, [muclight]),
-    escalus:create_users(Config1, escalus:get_users([alice, bob, kate, mike]));
+    escalus:create_users(Config1, escalus:get_users([alice, alice_bis, bob, kate, mike]));
 init_per_group(muc, Config) ->
     muc_helper:load_muc(),
     inbox_helper:reload_inbox_option(Config, groupchat, [muc]);
@@ -233,15 +244,17 @@ end_per_group(muclight_config, Config) ->
     muc_light_helper:clear_db(),
     HostType = domain_helper:host_type(mim),
     dynamic_modules:stop(HostType, mod_muc_light),
-    escalus:delete_users(Config, escalus:get_users([alice, bob, kate, mike]));
+    escalus:delete_users(Config, escalus:get_users([alice, alice_bis, bob, kate, mike]));
 end_per_group(_GroupName, Config) ->
     Config.
 
 
-init_per_testcase(create_groupchat_no_affiliation_stored, Config) ->
+init_per_testcase(TS, Config)
+  when TS =:= create_groupchat_no_affiliation_stored;
+       TS =:= system_message_is_correctly_avoided ->
     clear_inbox_all(),
     inbox_helper:reload_inbox_option(Config, aff_changes, false),
-    escalus:init_per_testcase(create_groupchat_no_affiliation_stored, Config);
+    escalus:init_per_testcase(TS, Config);
 init_per_testcase(leave_and_store_conversation, Config) ->
     clear_inbox_all(),
     inbox_helper:reload_inbox_option(Config, remove_on_kicked, false),
@@ -261,10 +274,12 @@ init_per_testcase(no_stored_and_remain_after_kicked, Config) ->
 init_per_testcase(CaseName, Config) ->
     escalus:init_per_testcase(CaseName, Config).
 
-end_per_testcase(create_groupchat_no_affiliation_stored, Config) ->
+end_per_testcase(TS, Config)
+  when TS =:= create_groupchat_no_affiliation_stored;
+       TS =:= system_message_is_correctly_avoided ->
     clear_inbox_all(),
     inbox_helper:restore_inbox_option(Config),
-    escalus:end_per_testcase(create_groupchat_no_affiliation_stored, Config);
+    escalus:end_per_testcase(TS, Config);
 end_per_testcase(leave_and_store_conversation, Config) ->
     clear_inbox_all(),
     inbox_helper:restore_inbox_option(Config),
@@ -1015,6 +1030,38 @@ groupchat_markers_all_reset_room_created(Config) ->
         inbox_helper:foreach_check_inbox([Bob, Kate, Alice], 0, AliceRoomJid, Msg)
       end).
 
+system_message_is_correctly_avoided(Config) ->
+    escalus:story(Config, [{alice, 1}, {alice_bis, 1}, {bob, 1}], fun(Alice, AliceBis, Bob) ->
+        %% Variables
+        Id = <<"MyID">>,
+        Msg1 = <<"Hi Room!">>,
+        Msg2 = <<"How are you?">>,
+        Users = [Alice, AliceBis, Bob],
+        InitOccupants = [{M, member} || M <- [AliceBis, Bob]],
+        Room = atom_to_binary(?FUNCTION_NAME, utf8),
+        BobJid = inbox_helper:to_bare_lower(Bob),
+        AliceJid = inbox_helper:to_bare_lower(Alice),
+        AliceBisJid = inbox_helper:to_bare_lower(AliceBis),
+        RoomJid = room_bin_jid(Room),
+        %% Given a room
+        muc_light_helper:given_muc_light_room(Room, Alice, InitOccupants),
+        BobRoomJid = <<RoomJid/binary,"/", BobJid/binary>>,
+        Stanza1 = escalus_stanza:set_id(escalus_stanza:groupchat_to(RoomJid, Msg1), Id),
+        %% Alice sends msg to room
+        escalus:send(Alice, Stanza1),
+        [ escalus:assert(is_groupchat_message, escalus:wait_for_stanza(User)) || User <- Users],
+        %% Bob sends second message
+        Stanza2 = escalus_stanza:set_id(escalus_stanza:groupchat_to(RoomJid, Msg2), Id),
+        escalus:send(Bob, Stanza2),
+        [ escalus:assert(is_groupchat_message, escalus:wait_for_stanza(User)) || User <- Users],
+        %% Alice has one unread message (from Bob)
+        check_inbox(Alice, [#conv{unread = 1, from = BobRoomJid, to = AliceJid, content = Msg2}]),
+        %% Bob has 0 unread messages because he sent the last message
+        check_inbox(Bob, [#conv{unread = 0, from = BobRoomJid, to = BobJid, content = Msg2}]),
+        %% AliceBis has 2 unread messages (from Alice and Bob) and does not have the affiliation
+        check_inbox(AliceBis, [#conv{unread = 2, from = BobRoomJid, to = AliceBisJid, content = Msg2}])
+      end).
+
 %%--------------------------------------------------------------------
 %% Classic MUC tests
 %%--------------------------------------------------------------------

src/inbox/mod_inbox.erl

@@ -30,7 +30,7 @@
 
 -export([process_iq/5,
          user_send_packet/4,
-         filter_packet/1,
+         filter_local_packet/1,
          inbox_unread_count/2,
          remove_user/3,
          remove_domain/3,
@@ -43,7 +43,7 @@
     {?MOD_INBOX_BACKEND, get_inbox, 4},
     {?MOD_INBOX_BACKEND, remove_domain, 2},
     {?MOD_INBOX_BACKEND, init, 2},
-    behaviour_info/1, disco_local_features/1, filter_packet/1, get_personal_data/3,
+    behaviour_info/1, disco_local_features/1, filter_local_packet/1, get_personal_data/3,
     inbox_unread_count/2, remove_domain/3, remove_user/3, user_send_packet/4
 ]).
 
@@ -266,10 +266,10 @@ inbox_unread_count(Acc, To) ->
                     To :: jid:jid(),
                     Acc :: mongoose_acc:t(),
                     Packet :: exml:element()}.
--spec filter_packet(Value :: fpacket() | drop) -> fpacket() | drop.
-filter_packet(drop) ->
+-spec filter_local_packet(Value :: fpacket() | drop) -> fpacket() | drop.
+filter_local_packet(drop) ->
     drop;
-filter_packet({From, To, Acc, Msg = #xmlel{name = <<"message">>}}) ->
+filter_local_packet({From, To, Acc, Msg = #xmlel{name = <<"message">>}}) ->
     %% In case of PgSQL we can we can update inbox and obtain unread_count in one query,
     %% so we put it in accumulator here.
     %% In case of MySQL/MsSQL it costs an extra query, so we fetch it only if necessary
@@ -282,7 +282,7 @@ filter_packet({From, To, Acc, Msg = #xmlel{name = <<"message">>}}) ->
            end,
     {From, To, Acc0, Msg};
 
-filter_packet({From, To, Acc, Packet}) ->
+filter_local_packet({From, To, Acc, Packet}) ->
     {From, To, Acc, Packet}.
 
 remove_user(Acc, User, Server) ->
@@ -587,7 +587,7 @@ hooks(HostType) ->
      {remove_user, HostType, ?MODULE, remove_user, 50},
      {remove_domain, HostType, ?MODULE, remove_domain, 50},
      {user_send_packet, HostType, ?MODULE, user_send_packet, 70},
-     {filter_local_packet, HostType, ?MODULE, filter_packet, 90},
+     {filter_local_packet, HostType, ?MODULE, filter_local_packet, 90},
      {inbox_unread_count, HostType, ?MODULE, inbox_unread_count, 80},
      {get_personal_data, HostType, ?MODULE, get_personal_data, 50},
      {disco_local_features, HostType, ?MODULE, disco_local_features, 99}

src/inbox/mod_inbox_muclight.erl

@@ -142,19 +142,10 @@ write_to_inbox(HostType, RoomUser, Remote, _Sender, Packet, Acc) ->
                          Sender :: jid:jid(),
                          Receiver :: jid:jid(),
                          Packet :: exml:element()) -> boolean().
-is_system_message(HostType, Sender, Receiver, Packet) ->
-    ReceiverDomain = Receiver#jid.lserver,
-    MUCLightDomain = mod_muc_light:server_host_to_muc_host(HostType, ReceiverDomain),
-    case {Sender#jid.lserver, Sender#jid.lresource} of
-        {MUCLightDomain, <<>>} ->
-            true;
-        {MUCLightDomain, _RoomUser} ->
-            false;
-        _Other ->
-            ?LOG_WARNING(#{what => inbox_muclight_unknown_message, packet => Packet,
-                           sender => jid:to_binary(Sender), receiver => jid:to_binary(Receiver)})
-    end.
-
+is_system_message(_HostType, #jid{lresource = <<>>}, _Receiver, _Packet) ->
+    true;
+is_system_message(_HostType, #jid{lresource = _RoomUser}, _Receiver, _Packet) ->
+    false.
 
 -spec is_change_aff_message(jid:jid(), exml:element(), role()) -> boolean().
 is_change_aff_message(User, Packet, Role) ->
@@ -176,7 +167,7 @@ system_message_type(User, Packet) ->
             kick;
        true ->
             other
-            end.
+    end.
 
 -spec is_invitation_message(jid:jid(), exml:element()) -> boolean().
 is_invitation_message(User, Packet) ->

src/muc_light/mod_muc_light.erl

@@ -84,7 +84,7 @@
     default_config/1, default_schema_definition/0, disco_local_items/1,
     force_clear_from_ct/0, is_muc_room_owner/4, prevent_service_unavailable/4,
     process_iq_get/5, process_iq_set/4, remove_domain/3, remove_user/3,
-    set_module_opt_from_ct/3
+    set_module_opt_from_ct/3, server_host_to_muc_host/2
 ]).
 
 -type muc_server() :: jid:lserver().