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.

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

GraphQL - Implement account queries #3503

Merged
merged 17 commits into from
Jan 27, 2022
Merged

Conversation

Premwoik
Copy link
Contributor

@Premwoik Premwoik commented Jan 17, 2022

This PR addresses MIM-1580 and adds account API for admin and user.

The old API functions have been extracted from mod_commands, ejabberd_admin and service_admin_extra_accounts modules to new mongoose_account_api module. Now old API modules use mongoose_account_api what standardized the result. The resulting account API returns a formatted message, so I didn't need to specify pretty error messages especially for GraphQL.Unfortunately, it does not follow the error message formatting convention in mongoose_graphql_errors.

Maybe because of extracting and reusing the old API, everything is more complicated now? Please tell me what do you think about it.

@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #3503 (f2c450f) into feature/graphql (4bd0ea2) will increase coverage by 0.06%.
The diff coverage is 95.37%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           feature/graphql    #3503      +/-   ##
===================================================
+ Coverage            81.12%   81.19%   +0.06%     
===================================================
  Files                  439      446       +7     
  Lines                32660    32774     +114     
===================================================
+ Hits                 26497    26612     +115     
+ Misses                6163     6162       -1     
Impacted Files Coverage Δ
src/graphql/mongoose_graphql.erl 91.11% <ø> (ø)
src/graphql/user/mongoose_graphql_user_query.erl 50.00% <0.00%> (-50.00%) ⬇️
src/mongoose_session_api.erl 66.66% <66.66%> (ø)
src/mod_commands.erl 92.91% <84.61%> (+0.94%) ⬆️
src/admin_extra/service_admin_extra_accounts.erl 92.30% <91.66%> (+1.64%) ⬆️
.../admin/mongoose_graphql_account_admin_mutation.erl 96.15% <96.15%> (ø)
src/mongoose_account_api.erl 97.39% <97.39%> (ø)
src/admin_extra/service_admin_extra_sessions.erl 94.00% <100.00%> (+4.90%) ⬆️
src/ejabberd_admin.erl 57.28% <100.00%> (-1.06%) ⬇️
...hql/admin/mongoose_graphql_account_admin_query.erl 100.00% <100.00%> (ø)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bd0ea2...f2c450f. Read the comment docs.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@Premwoik Premwoik force-pushed the graphql/accounts-api branch 2 times, most recently from 26fa3ec to 9c6914c Compare January 19, 2022 08:31
@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@Premwoik Premwoik removed the WIP 🚧 label Jan 19, 2022
@Premwoik Premwoik marked this pull request as ready for review January 19, 2022 10:02
Copy link
Contributor

@arcusfelis arcusfelis left a comment

Choose a reason for hiding this comment

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

comments, nice progress :)

big_tests/tests/graphql_account_SUITE.erl Outdated Show resolved Hide resolved
big_tests/tests/graphql_account_SUITE.erl Outdated Show resolved Hide resolved
big_tests/tests/graphql_account_SUITE.erl Outdated Show resolved Hide resolved
big_tests/tests/graphql_account_SUITE.erl Outdated Show resolved Hide resolved
big_tests/tests/graphql_account_SUITE.erl Outdated Show resolved Hide resolved

-spec set_random_password(JID, Reason) -> Result when
JID :: jid:jid(),
Reason :: binary(),
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we need to define a type for Reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What it should be? -type reason() :: binary().?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes :)

src/mongoose_account_api.erl Outdated Show resolved Hide resolved
io_lib:format("~4..0w-~2..0w-~2..0wT~2..0w:~2..0w:~2..0wZ",
[Year, Month, Day, Hour, Minute, Second]))),
RandomString = mongoose_bin:gen_from_crypto(),
<<"BANNED_ACCOUNT--", Date/binary, "--", RandomString/binary, "--", Reason/binary>>.
Copy link
Contributor

Choose a reason for hiding this comment

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

That is kinda evil way to ban users, lol. Haaack.
That should be documented somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to document it, but I think this falls out of scope of this PR as the code is unchanged here.

src/mongoose_account_api.erl Outdated Show resolved Hide resolved
src/mongoose_account_api.erl Show resolved Hide resolved
@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im

This comment has been minimized.

@mongoose-im
Copy link
Collaborator

mongoose-im commented Jan 25, 2022

small_tests_24 / small_tests / c1e8dbd
Reports root / small


small_tests_23 / small_tests / c1e8dbd
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / c1e8dbd
Reports root/ big
OK: 2748 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / c1e8dbd
Reports root/ big
OK: 2748 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / c1e8dbd
Reports root/ big
OK: 2731 / Failed: 0 / User-skipped: 255 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / c1e8dbd
Reports root/ big
OK: 1525 / Failed: 0 / User-skipped: 422 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / c1e8dbd
Reports root/ big
OK: 2748 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / c1e8dbd
Reports root/ big
OK: 1525 / Failed: 0 / User-skipped: 422 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / c1e8dbd
Reports root/ big
OK: 1571 / Failed: 0 / User-skipped: 376 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / c1e8dbd
Reports root/ big
OK: 3135 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / c1e8dbd
Reports root/ big
OK: 3135 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / c1e8dbd
Reports root/ big
OK: 3135 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / c1e8dbd
Reports root/ big
OK: 1877 / Failed: 0 / User-skipped: 377 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / c1e8dbd
Reports root/ big
OK: 1723 / Failed: 0 / User-skipped: 377 / Auto-skipped: 0

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Lots of good work done here! I only have very minor remarks.

priv/graphql/schemas/admin/account.gql Outdated Show resolved Hide resolved
src/mongoose_session_api.erl Outdated Show resolved Hide resolved
@mongoose-im
Copy link
Collaborator

mongoose-im commented Jan 27, 2022

small_tests_24 / small_tests / f2c450f
Reports root / small


small_tests_23 / small_tests / f2c450f
Reports root / small


internal_mnesia_24 / internal_mnesia / f2c450f
Reports root


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / f2c450f
Reports root/ big
OK: 2748 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / f2c450f
Reports root/ big
OK: 2748 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / f2c450f
Reports root/ big
OK: 2731 / Failed: 0 / User-skipped: 255 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / f2c450f
Reports root/ big
OK: 2748 / Failed: 0 / User-skipped: 238 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / f2c450f
Reports root/ big
OK: 1525 / Failed: 0 / User-skipped: 422 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / f2c450f
Reports root/ big
OK: 1525 / Failed: 0 / User-skipped: 422 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / f2c450f
Reports root/ big
OK: 3135 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / f2c450f
Reports root/ big
OK: 3135 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / f2c450f
Reports root/ big
OK: 1877 / Failed: 0 / User-skipped: 377 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / f2c450f
Reports root/ big
OK: 3130 / Failed: 0 / User-skipped: 252 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / f2c450f
Reports root/ big
OK: 3135 / Failed: 0 / User-skipped: 247 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / f2c450f
Reports root/ big
OK: 1723 / Failed: 0 / User-skipped: 377 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / f2c450f
Reports root


internal_mnesia_24 / internal_mnesia / f2c450f
Reports root

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

It's all good for me!

@arcusfelis arcusfelis merged commit 6b0e7e9 into feature/graphql Jan 27, 2022
@arcusfelis arcusfelis deleted the graphql/accounts-api branch January 27, 2022 15:21
@chrzaszcz chrzaszcz added this to the 6.0.0 milestone Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants