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

Admin extra interface #575

Merged
merged 6 commits into from
Dec 3, 2015
Merged

Admin extra interface #575

merged 6 commits into from
Dec 3, 2015

Conversation

ludwikbukowski
Copy link
Contributor

I tried to make admin interface more hospitable. It should do some checks on input and inform about result of command.
Improved following modules:

  • mod_admin_extra_accounts
  • mod_admin_extra_last
  • mod_admin_extra_private
  • mod_admin_extra_roster
  • mod_admin_extra_stanza
  • mod_admin_extra_stats
  • mod_admin_extra_vcard

@michalwski
Copy link
Contributor

@ludwikbukowski sth's wrong with ejabberdctl_SUITE:vcard_rw because all jobs failed except the one using external authentication (where this test case is probably not run).

(_M, Res) ->
Res
end, {error, not_allowed}, auth_modules(LServer)).
case is_user_exists(LUser, LServer) of
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this check should be moved to the main function which is set_password unless do_set_password is used directly from other places.

@@ -113,21 +113,44 @@ commands() ->
%%%

get_vcard(User, Host, Name) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember to add specs for exported functions and also change specs when you modify functions

@ludwikbukowski ludwikbukowski force-pushed the admin_extra_interface branch 4 times, most recently from c27cc77 to 6d5cb9b Compare November 16, 2015 19:18
@michalwski
Copy link
Contributor

I see that most (if not all) commands you touched have now result type {res, restuple}. This is correct, but not much informative. How about spending some more time and defining more precise result types. A good example maybe command connected_users_info. Which you can find in mod_admin_extra_sessions module. The result type for this command looks like the following (in shell).

mongooseimctl help connected_users_info

  Command Name: connected_users_info

  Arguments:

  Returns: connected_users_info::[ sessions::{ jid::string,
                                               connection::string,
                                               ip::string,
                                               port::integer,
                                               priority::integer,
                                               node::string,
                                               uptime::integer } ]

  Tags: session

  Description: List all established sessions and their information

Also I think we can remove the unnecessary empty lines in the help output above.

michalwski added a commit that referenced this pull request Dec 3, 2015
@michalwski michalwski merged commit b02b0b1 into master Dec 3, 2015
@michalwski michalwski deleted the admin_extra_interface branch December 3, 2015 08:19
@michalwski michalwski mentioned this pull request Dec 4, 2015
"Subj", "\"Are
you there?\""],
Config),
Err1 =/=0,
Copy link
Member

Choose a reason for hiding this comment

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

I noticed this on Travis when testing stuff rebased on top of this PR:

ejabberdctl_SUITE.erl:750: Warning: use of operator '=/=' has no effect
ejabberdctl_SUITE.erl:751: Warning: use of operator '=/=' has no effect

Which makes sense, since these lines are not assertions! They evaluate to a boolean() which is then discarded.

Please rewrite to:

true = Err1 =/= 0

or

?assertNotEqual(0, Err1)

or

?assert(Err1 =/= 0)

@ludwikbukowski ludwikbukowski restored the admin_extra_interface branch February 29, 2016 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants