HTTP client API - improve room users API #982

michalwski · 2016-09-13T10:12:24Z

This PR goes on top of #973

Proposed changes include:

user's management move to path /room/:id/users
id attribute to affiliation change messages initiated by the API

room's user management is now available on path /rooms[:id]/users

user can leave a room by deletine itself from it

gadgetci · 2016-09-22T07:48:53Z

test/ejabberd_tests/tests/rest_client_SUITE.erl

+        RoomID = given_new_room_with_users({alice, Alice}, [{bob, Bob}]),
+        {{<<"204">>, _}, _} = remove_user_from_a_room({alice, Alice}, RoomID, Alice),
+        Stanza = escalus:wait_for_stanza(Bob),
+        ct:pal("~p", [Stanza]),


According to Elvis:

Remove the debug call to ct:pal/2 on line 151.

gadgetci · 2016-09-22T07:48:53Z

test/ejabberd_tests/tests/rest_client_SUITE.erl

+user_is_removed_from_a_room(Config) ->
+    escalus:fresh_story(Config, [{alice, 1}, {bob, 1}], fun(Alice, Bob) ->
+        RoomID = given_new_room_with_users({alice, Alice}, [{bob, Bob}]),
+        {{<<"204">>, _}, _} = remove_user_from_a_room({alice, Alice}, RoomID, Bob),


According to Elvis:

The code in the following (LINE, COL) locations has the same structure: (141, 29), (149, 29).

gadgetci · 2016-09-22T07:48:54Z

test/ejabberd_tests/tests/rest_client_SUITE.erl

@@ -116,13 +119,46 @@ messages_can_be_paginated(Config) ->
 room_is_created(Config) ->
    escalus:fresh_story(Config, [{alice, 1}], fun(Alice) ->
        RoomID = given_new_room({alice, Alice}),
-        get_room_info({alice, Alice}, RoomID)
+        RoomInfo = get_room_info({alice, Alice}, RoomID),
+        assert_room_info(Alice, RoomInfo)
    end).

 user_is_invited_to_a_room(Config) ->
    escalus:fresh_story(Config, [{alice, 1}, {bob, 1}], fun(Alice, Bob) ->
        RoomID = given_new_room_with_users({alice, Alice}, [{bob, Bob}]),


According to Elvis:

The code in the following (LINE, COL) locations has the same structure: (128, 16), (140, 16), (148, 16), (158, 16), (202, 16).

gadgetci · 2016-09-22T07:48:55Z

apps/ejabberd/src/mod_muc_light_admin.erl

@@ -127,14 +127,14 @@ invite_to_room(Domain, RoomName, Sender, Recipient0) ->
    ejabberd_router:route(S, R, iq(jid:to_binary(S), jid:to_binary(R),


According to Elvis:

The code in the following (LINE, COL) locations has the same structure: (127, 20), (138, 20).

michalwski · 2016-09-22T08:33:58Z

All the code duplicates where addressed by tuning the Elvis's DRY sensitivity.

bartekgorny · 2016-09-22T10:32:00Z

apps/ejabberd/src/mod_muc_light_admin.erl

@@ -210,10 +210,12 @@ is_subdomain(Child, Parent) ->

 iq(S, R, T, C) when is_binary(S),
                    is_binary(R), is_binary(T), is_list(C) ->
+    UUID = uuid:uuid_to_string(uuid:get_v4(), binary_standard),


I'd rather call it Uuid, to keep casing consistent.

I'm actually for UUID as it is not only var name but also shortcut for Universal Unique Identifier which in many places is referred as UUID. Also to me UUID is more readable then Uuid.

bartekgorny · 2016-09-22T10:33:05Z

apps/ejabberd/src/mod_muc_light_admin.erl

@@ -210,10 +210,12 @@ is_subdomain(Child, Parent) ->

 iq(S, R, T, C) when is_binary(S),


Some more telling variable names would be useful.

I agree, I forgot to refactor this function.

bartekgorny

General remark: this approach gives more control over urls and allows producing nicer paths, but results in many modules and code duplication. Some of it could be avoided, because we have subcategories in commands, so:
/api/messages/[:with]
/api/rooms/[:id]
/api/rooms/[:id]/messages
could well be served by the same module.
Indeed, this:
/api/rooms/:id/users/[:user]
could not be auto-generated, because using subcategory you could only get:
/api/rooms/:id/[:user]/users
which doesn't make much sense. I'd suggest looking for a way to route multiple urls to the same module, so as not to add a module for every simple function.

michalwski · 2016-09-22T11:32:52Z

What do you consider code duplication here?
It's very easy to route multiple urls to the same module, but then we'll end up in request handlers where we have to check the path, and method and whatnot before we decide what to do. When we separate the whole rooms' functionality to smaller modules implementing only one logically small subset of functionality, the implementation is more understandable but a bit longer.

bartekgorny · 2016-09-22T12:26:47Z

I mean modules like the mongoose_client_api_rooms_users, which mostly calls mongoose_client_api_rooms and adds little of its own logic. But it is a matter of taste. I'm a bit concerned about numer of modules, it is growing very fast, and there is already quite a lot of them.

michalwski · 2016-09-22T13:46:26Z

This could be improved by using inaka/mixer. I'd rather have more modules where the functionality is clearly split and each module is responsible for one logical subset.
In the end we can organise all the REST modules into their own subdirectory or even separate mongooseim_rest app in apps dir.

bartekgorny · 2016-09-23T08:30:49Z

Sounds like a good idea. Anyway, as of today, that's all from me.

michalwski · 2016-09-23T08:36:01Z

If so then let's merge the PR and move forward!

michalwski changed the title ~~Http client api improve room users api~~ HTTP client API - improve room users API Sep 13, 2016

michalwski added 4 commits September 14, 2016 11:34

[REST API] move room's users to a separate handler

704c911

room's user management is now available on path /rooms[:id]/users

[REST API] implement method for removing users from a room

2c3ae89

[REST API] implement user leaving from a room

4cb3e1d

user can leave a room by deletine itself from it

use assert_aff_change to verify invitation

fb201af

michalwski force-pushed the http-client-api-improve-room-users-api branch from 4b114ec to 144e5fd Compare September 14, 2016 09:34

michalwski added 2 commits September 14, 2016 12:15

[REST API] small typo fixes in swagger spec [skip ci]

de83475

[REST API] ensure the id attr is added to aff change message

985eccf

michalwski force-pushed the http-client-api-improve-room-users-api branch from 144e5fd to 985eccf Compare September 14, 2016 10:15

michalwski added 2 commits September 15, 2016 13:30

[REST API] fix swagger spec - users are invited via POST method

15205f0

[REST API] improve tests for room invitations

6467e64

michalwski force-pushed the http-client-api-improve-room-users-api branch from 6f84653 to 939b5b1 Compare September 22, 2016 07:28

michalwski added 2 commits September 22, 2016 09:46

add evlis.config to override default line length rule

2807a9a

[REST API] add test where owner leaves a room

892af90

michalwski force-pushed the http-client-api-improve-room-users-api branch from 939b5b1 to 892af90 Compare September 22, 2016 07:48

gadgetci reviewed Sep 22, 2016

View reviewed changes

michalwski added 2 commits September 22, 2016 10:06

tune Elvis's DRY

4bc5b66

improve code following @gadgetci Elvis's comments

883ebc2

michalwski added the ready label Sep 22, 2016

bartekgorny reviewed Sep 22, 2016

View reviewed changes

change some parameter names to be self-descriptive

e5204b5

bartekgorny approved these changes Sep 23, 2016

View reviewed changes

bartekgorny merged commit 3c0fa40 into master Sep 23, 2016

bartekgorny deleted the http-client-api-improve-room-users-api branch September 23, 2016 08:37

michalwski mentioned this pull request Nov 8, 2016

MongooseIM 2.0.0 #1069

Merged

Nyco removed the ready label Nov 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP client API - improve room users API #982

HTTP client API - improve room users API #982

michalwski commented Sep 13, 2016 •

edited

Loading

gadgetci Sep 22, 2016

gadgetci Sep 22, 2016

gadgetci Sep 22, 2016

gadgetci Sep 22, 2016

michalwski commented Sep 22, 2016

bartekgorny Sep 22, 2016

michalwski Sep 22, 2016

bartekgorny Sep 22, 2016

michalwski Sep 22, 2016

bartekgorny left a comment

michalwski commented Sep 22, 2016

bartekgorny commented Sep 22, 2016

michalwski commented Sep 22, 2016

bartekgorny commented Sep 23, 2016

michalwski commented Sep 23, 2016

		@@ -127,14 +127,14 @@ invite_to_room(Domain, RoomName, Sender, Recipient0) ->
		ejabberd_router:route(S, R, iq(jid:to_binary(S), jid:to_binary(R),

		@@ -210,10 +210,12 @@ is_subdomain(Child, Parent) ->

		iq(S, R, T, C) when is_binary(S),

HTTP client API - improve room users API #982

HTTP client API - improve room users API #982

Conversation

michalwski commented Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekgorny left a comment

Choose a reason for hiding this comment

michalwski commented Sep 22, 2016

bartekgorny commented Sep 22, 2016

michalwski commented Sep 22, 2016

bartekgorny commented Sep 23, 2016

michalwski commented Sep 23, 2016

michalwski commented Sep 13, 2016 •

edited

Loading