Improve error handling in API module last

big_tests/tests/graphql_last_SUITE.erl

@@ -5,15 +5,18 @@
 -import(distributed_helper, [mim/0, require_rpc_nodes/1, rpc/4]).
 -import(graphql_helper, [execute_command/4, execute_user_command/5, user_to_bin/1, user_to_jid/1,
                          get_ok_value/2, get_err_msg/1, get_err_code/1, get_unauthorized/1,
-                         get_not_loaded/1]).
+                         get_not_loaded/1, get_coercion_err_msg/1]).
 
 -include_lib("eunit/include/eunit.hrl").
 
 -define(assertErrMsg(Res, ContainsPart), assert_err_msg(ContainsPart, Res)).
 -define(assertErrCode(Res, Code), assert_err_code(Code, Res)).
 
 -define(NONEXISTENT_JID, <<"[email protected]">>).
+-define(NONEXISTENT_NAME, <<"user@", (domain_helper:domain())/binary>>).
+-define(EMPTY_NAME_JID, <<"@", (domain_helper:domain())/binary>>).
 -define(DEFAULT_DT, <<"2022-04-17T12:58:30.000000Z">>).
+-define(INVALID_TIMESTAMP, <<"20222-04-17T12:58:30.000000Z">>).
 -define(NONEXISTENT_DOMAIN, <<"nonexistent">>).
 
 suite() ->
@@ -73,33 +76,41 @@ user_last_not_configured() ->
 admin_last_tests() ->
     [admin_set_last,
      admin_try_set_nonexistent_user_last,
+     admin_try_set_last_invalid_timestamp,
      admin_get_last,
      admin_get_nonexistent_user_last,
      admin_try_get_nonexistent_last,
      admin_count_active_users,
-     admin_try_count_nonexistent_domain_active_users].
+     admin_try_count_nonexistent_domain_active_users,
+     admin_try_count_active_users_invalid_timestamp].
 
 admin_old_users_tests() ->
     [admin_list_old_users_domain,
      admin_try_list_old_users_nonexistent_domain,
+     admin_try_list_old_users_invalid_timestamp,
      admin_list_old_users_global,
      admin_remove_old_users_domain,
      admin_try_remove_old_users_nonexistent_domain,
+     admin_try_remove_old_users_invalid_timestamp,
      admin_remove_old_users_global,
      admin_user_without_last_info_is_old_user,
      admin_logged_user_is_not_old_user].
 
 domain_admin_last_tests() ->
     [admin_set_last,
      domain_admin_set_user_last_no_permission,
+     admin_try_set_last_invalid_timestamp,
      admin_get_last,
      domain_admin_get_user_last_no_permission,
      admin_try_get_nonexistent_last,
      admin_count_active_users,
-     domain_admin_try_count_external_domain_active_users].
+     domain_admin_try_count_external_domain_active_users,
+     admin_try_count_active_users_invalid_timestamp].
 
 domain_admin_old_users_tests() ->
     [admin_list_old_users_domain,
+     admin_try_list_old_users_invalid_timestamp,
+     admin_try_remove_old_users_invalid_timestamp,
      domain_admin_try_list_old_users_external_domain,
      domain_admin_list_old_users_global,
      domain_admin_remove_old_users_global,
@@ -244,7 +255,20 @@ admin_set_last_story(Config, Alice) ->
 admin_try_set_nonexistent_user_last(Config) ->
     Res = admin_set_last(?NONEXISTENT_JID, <<"status">>, null, Config),
     ?assertErrMsg(Res, <<"not exist">>),
-    ?assertErrCode(Res, user_does_not_exist).
+    ?assertErrCode(Res, user_does_not_exist),
+    Res2 = admin_set_last(?NONEXISTENT_NAME, <<"status">>, null, Config),
+    ?assertErrMsg(Res2, <<"not exist">>),
+    ?assertErrCode(Res2, user_does_not_exist),
+    Res3 = admin_set_last(?EMPTY_NAME_JID, <<"status">>, null, Config),
+    get_coercion_err_msg(Res3).
+
+admin_try_set_last_invalid_timestamp(Config) ->
+    escalus:fresh_story_with_config(Config, [{alice, 1}],
+                                    fun admin_try_set_last_invalid_timestamp_story/2).
+
+admin_try_set_last_invalid_timestamp_story(Config, Alice) ->
+    Res = admin_set_last(Alice, <<"status">>, ?INVALID_TIMESTAMP, Config),
+    get_coercion_err_msg(Res).
 
 admin_get_last(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}],
@@ -261,7 +285,12 @@ admin_get_last_story(Config, Alice) ->
 admin_get_nonexistent_user_last(Config) ->
     Res = admin_get_last(?NONEXISTENT_JID, Config),
     ?assertErrMsg(Res, <<"not exist">>),
-    ?assertErrCode(Res, user_does_not_exist).
+    ?assertErrCode(Res, user_does_not_exist),
+    Res2 = admin_get_last(?NONEXISTENT_NAME, Config),
+    ?assertErrMsg(Res2, <<"not exist">>),
+    ?assertErrCode(Res2, user_does_not_exist),
+    Res3 = admin_get_last(?EMPTY_NAME_JID, Config),
+    get_coercion_err_msg(Res3).
 
 admin_try_get_nonexistent_last(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}],
@@ -290,6 +319,11 @@ admin_try_count_nonexistent_domain_active_users(Config) ->
     ?assertErrMsg(Res, <<"not found">>),
     ?assertErrCode(Res, domain_not_found).
 
+admin_try_count_active_users_invalid_timestamp(Config) ->
+    Domain = domain_helper:domain(),
+    Res = admin_count_active_users(Domain, ?INVALID_TIMESTAMP, Config),
+    get_coercion_err_msg(Res).
+
 %% Admin old users test cases
 
 admin_remove_old_users_domain(Config) ->
@@ -315,6 +349,11 @@ admin_try_remove_old_users_nonexistent_domain(Config) ->
     ?assertErrMsg(Res, <<"not found">>),
     ?assertErrCode(Res, domain_not_found).
 
+admin_try_remove_old_users_invalid_timestamp(Config) ->
+    Domain = domain_helper:domain(),
+    Res = admin_remove_old_users(Domain, ?INVALID_TIMESTAMP, Config),
+    get_coercion_err_msg(Res).
+
 admin_remove_old_users_global(Config) ->
     jids_with_config(Config, [alice, alice_bis, bob], fun admin_remove_old_users_global_story/4).
 
@@ -355,6 +394,11 @@ admin_try_list_old_users_nonexistent_domain(Config) ->
     ?assertErrMsg(Res, <<"not found">>),
     ?assertErrCode(Res, domain_not_found).
 
+admin_try_list_old_users_invalid_timestamp(Config) ->
+    Domain = domain_helper:domain(),
+    Res = admin_list_old_users(Domain, ?INVALID_TIMESTAMP, Config),
+    get_coercion_err_msg(Res).
+
 admin_list_old_users_global(Config) ->
     jids_with_config(Config, [alice, alice_bis, bob], fun admin_list_old_users_global_story/4).
 

src/admin_extra/service_admin_extra_accounts.erl

@@ -133,7 +133,7 @@ num_active_users(Domain, Days) ->
 -spec delete_old_users(integer()) -> {ok, iolist()}.
 delete_old_users(Days) ->
     Timestamp = days_to_timestamp(Days),
-    OldUsers = mod_last_api:remove_old_users(Timestamp),
+    {ok, OldUsers} = mod_last_api:remove_old_users(Timestamp),
     {ok, format_deleted_users(OldUsers)}.
 
 -spec delete_old_users_for_domain(jid:server(), integer()) -> {ok | domain_not_found, iolist()}.

src/graphql/admin/mongoose_graphql_last_admin_mutation.erl

@@ -26,7 +26,7 @@ set_last(#{<<"user">> := JID, <<"timestamp">> := Timestamp, <<"status">> := Stat
 -spec remove_old_users(args()) -> {ok, old_users()} | {error, resolver_error()}.
 remove_old_users(#{<<"domain">> := null, <<"timestamp">> := Timestamp}) ->
     Timestamp2 = mongoose_graphql_last_helper:microseconds_to_seconds(Timestamp),
-    OldUsers = mod_last_api:remove_old_users(Timestamp2),
+    {ok, OldUsers} = mod_last_api:remove_old_users(Timestamp2),
     {ok, format_old_users(OldUsers)};
 remove_old_users(#{<<"domain">> := Domain, <<"timestamp">> := Timestamp}) ->
     Timestamp2 = mongoose_graphql_last_helper:microseconds_to_seconds(Timestamp),

src/graphql/admin/mongoose_graphql_last_admin_query.erl

@@ -35,7 +35,7 @@ count_active_users(#{<<"domain">> := Domain, <<"timestamp">> := Timestamp}) ->
 -spec list_old_users(args()) -> {ok, old_users()} | {error, resolver_error()}.
 list_old_users(#{<<"domain">> := null, <<"timestamp">> := Timestamp}) ->
     Timestamp2 = mongoose_graphql_last_helper:microseconds_to_seconds(Timestamp),
-    OldUsers = mod_last_api:list_old_users(Timestamp2),
+    {ok, OldUsers} = mod_last_api:list_old_users(Timestamp2),
     {ok, format_old_users(OldUsers)};
 list_old_users(#{<<"domain">> := Domain, <<"timestamp">> := Timestamp}) ->
     Timestamp2 = mongoose_graphql_last_helper:microseconds_to_seconds(Timestamp),

src/mod_last_api.erl

@@ -57,11 +57,12 @@ count_active_users(Domain, Timestamp) ->
             ?DOMAIN_NOT_FOUND_RESULT
     end.
 
--spec list_old_users(timestamp()) -> [old_user()].
+-spec list_old_users(timestamp()) -> {ok, [old_user()]}.
 list_old_users(Timestamp) ->
-    lists:append([list_old_users(HostType, Domain, Timestamp) ||
-                  HostType <- ?ALL_HOST_TYPES,
-                  Domain <- mongoose_domain_api:get_domains_by_host_type(HostType)]).
+    OldUsers = lists:append([list_old_users(HostType, Domain, Timestamp) ||
+                             HostType <- ?ALL_HOST_TYPES,
+                             Domain <- mongoose_domain_api:get_domains_by_host_type(HostType)]),
+    {ok, OldUsers}.
 
 -spec list_old_users(jid:server(), timestamp()) ->
     {ok, [old_user()]} | {domain_not_found, binary()}.
@@ -74,11 +75,11 @@ list_old_users(Domain, Timestamp) ->
             ?DOMAIN_NOT_FOUND_RESULT
     end.
 
--spec remove_old_users(timestamp()) -> [old_user()].
+-spec remove_old_users(timestamp()) -> {ok, [old_user()]}.
 remove_old_users(Timestamp) ->
-    OldUsers = list_old_users(Timestamp),
+    {ok, OldUsers} = list_old_users(Timestamp),
     ok = remove_users(OldUsers),
-    OldUsers.
+    {ok, OldUsers}.
 
 -spec remove_old_users(jid:server(), timestamp()) ->
     {ok, [old_user()]} | {domain_not_found, binary()}.