Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Inbox-muclight: fix system messages #3228

Merged
merged 6 commits into from
Aug 25, 2021
Merged

Inbox-muclight: fix system messages #3228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5658da7
Rename handler to avoid confusing with filter_packet global hook
NelsonVides Aug 20, 2021
5322568
Test inbox system messages under diverse domains
NelsonVides Aug 20, 2021
543be65
Remove retries from inbox
NelsonVides Aug 21, 2021
bd87c8f
system messages simply come from the room bare jid
NelsonVides Aug 23, 2021
3921575
fix xref
NelsonVides Aug 23, 2021
205d1e1
Apply review
NelsonVides Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
215 changes: 131 additions & 84 deletions big_tests/tests/inbox_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
]}
].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the removal of ct_helper:repeat_all_until_all_ok/1 intentional? Do you think we should remove it ad-hoc when touching any test suite?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very intentional, it annoys me to death. I've never seen any non-deterministic test in inbox, if we ever introduce anything asynchronously weird we can always insert a common-test repeat command for that specific test-case. I'd very strongly argue in favour of being very explicit about what is expected to misbehave and what is expected to be absolutely deterministic.

Copy link
Member

@chrzaszcz chrzaszcz Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I just thought about doing it separately for all test suites at once to avoid inconsistency and guessing if it is done or not done for a particular module, which would be confusing.


suite() ->
escalus:suite().
Expand All @@ -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) ->
Expand All @@ -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]);
Expand All @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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
%%--------------------------------------------------------------------
Expand Down
14 changes: 7 additions & 7 deletions src/inbox/mod_inbox.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
]).

Expand Down Expand Up @@ -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
Expand All @@ -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) ->
Expand Down Expand Up @@ -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}
Expand Down
19 changes: 5 additions & 14 deletions src/inbox/mod_inbox_muclight.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) ->
Expand All @@ -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) ->
Expand Down
2 changes: 1 addition & 1 deletion src/muc_light/mod_muc_light.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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().
Expand Down