Add basic implementation for the tags endpoint. #105

farodin91 · 2016-10-12T19:17:52Z

No description provided.

farodin91 · 2016-10-15T12:43:13Z

@jimmycuadra Review?

farodin91 · 2016-10-15T12:52:13Z

I need to add an event m.tag.

farodin91 · 2016-10-15T13:01:01Z

This could done by different PR. It need to fix ruma/ruma-events#4, before this issue could resolve correct.

mujx · 2016-10-17T18:31:48Z

src/api/r0/tags.rs

+
+        // Check if the given user_id corresponds to the authenticated user.
+        if user_id != user.id {
+            Err(ApiError::unauthorized(None))?;


Could you add some useful error message instead of None? You can use this for this particular error, in order to be consistent.

mujx · 2016-10-17T18:47:53Z

src/tags.rs

+        room_id: RoomId,
+        tag: String,
+        content: Value)
+        -> Result<(), ApiError> {


Until we use something like rustfmt it would be better to stick with the current style.

mujx · 2016-10-17T21:17:45Z

src/api/r0/tags.rs

+
+        let mut content = String::new();
+        if let Err(_) = request.body.read_to_string(&mut content) {
+            Err(ApiError::not_found(None))?;


You missed this one. Although synapse parses it as json, so you can skip this check and just add the JsonRequest middleware.

Resolved!
I think the json middleware could be refactored in the future.

mujx · 2016-10-17T21:25:53Z

src/api/r0/tags.rs

+        let user = request.extensions.get::<User>()
+            .expect("AccessTokenAuth should ensure a user").clone();
+        let params = request.extensions.get::<Router>().expect("Params object is missing").clone();
+        let tag = match params.find("tag") {


You can implement this check as a middleware (e.g TagParam).

jimmycuadra · 2016-11-24T13:36:13Z

src/api/r0/tags.rs

+use tags::RoomTag;
+use user::User;
+
+pub type MapTags = BTreeMap<String, Value>;


Why BTreeMap over HashMap?

farodin91 · 2016-12-02T14:40:01Z

Waiting for #114

farodin91 · 2016-12-08T12:36:12Z

@jimmycuadra Could you start reviewing? Thanks

mujx · 2016-12-08T12:46:20Z

src/api/r0/tags.rs

+    #[test]
+    fn basic_create_tag() {
+        let test = Test::new();
+        let access_token = test.create_access_token(); // @carl:ruma.test


You can remove the comment by using the create_access_token_with_username("carl")

mujx · 2016-12-08T12:49:23Z

src/api/r0/tags.rs

+        let room_id = test.create_public_room(&access_token);
+        let put_tag_path = format!(
+            "/_matrix/client/r0/user/{}/rooms/{}/tags/{}?access_token={}",
+            "@carls:ruma.test",


Create another access token e.g alice. The user is not registered, so the action is not forbidden but the user id was not found.

I would do this after is fixed #121.

mujx · 2016-12-08T12:52:53Z

src/api/r0/tags.rs

+    }
+
+    #[test]
+    fn delete_tag_forbidden() {


Why is this forbidden?

mujx · 2016-12-08T12:58:36Z

src/models/tags.rs

+            .filter(room_tags::user_id.eq(user_id))
+            .get_results(connection)
+            .map_err(|err| match err {
+                DieselError::NotFound => ApiError::not_found(None),


You can use the err instead of None.

mujx · 2016-12-08T13:02:33Z

src/models/tags.rs

+        delete(tag)
+            .execute(connection)
+            .map_err(|err| match err {
+                DieselError::NotFound => ApiError::not_found(None),


Also a missing error message here.

mujx · 2016-12-08T13:06:40Z

src/models/tags.rs

+        user_id: UserId,
+        room_id: RoomId,
+        tag: String,
+        content: Value,


Why Value instead of String?

mujx · 2016-12-08T13:19:42Z

src/api/r0/tags.rs

+    }
+
+    #[test]
+    fn update_tag() {


You can add another test where you update a non-existent tag.

No, See Line 177, 179

Sorry, my bad. Rename the helper function to put_tag so there is no confusion.

mujx · 2016-12-08T13:21:56Z

src/api/r0/tags.rs

+    fn handle(&self, request: &mut Request) -> IronResult<Response> {
+        let user_id = request.extensions.get::<UserIdParam>()
+            .expect("UserIdParam should ensure a UserId").clone();
+        let room_id = request.extensions.get::<RoomIdParam>()


You need to check that the room and user the exist before adding a tag. Also add the relevant tests. We don't have foreign keys so there will be no error.

I'll only check room_id. The user is check by forbidden.

Solved for all three, but not nice for delete.

mujx · 2016-12-08T13:28:29Z

src/models/tags.rs

+            map.insert(tag.tag, info);
+        }
+
+        let tags = TagEventContent { tags: map };


Better use a custom TagsResponse in the api handler and here just return the hashmap.

jimmycuadra · 2016-12-21T06:42:57Z

Fantastic work. :D

farodin91 force-pushed the tags branch from 9931e99 to fdecdec Compare October 12, 2016 20:05

jimmycuadra added component/api type/feature spec/tags status/work in progress labels Oct 12, 2016

This was referenced Oct 12, 2016

API: PUT /user/:user_id/rooms/:room_id/tags/:tag #59

Closed

API: DELETE /user/:user_id/rooms/:room_id/tags/:tag #60

Closed

API: GET /user/:user_id/rooms/:room_id/tags #61

Closed

farodin91 force-pushed the tags branch from fdecdec to 9ac27d4 Compare October 15, 2016 12:18

farodin91 changed the title ~~[WIP] Add basic implementation for the tags endpoint.~~ Add basic implementation for the tags endpoint. Oct 15, 2016

farodin91 force-pushed the tags branch 3 times, most recently from 0d59ebc to 3d4e32f Compare October 15, 2016 12:42

farodin91 closed this Oct 15, 2016

farodin91 reopened this Oct 15, 2016

farodin91 added status/needs review and removed status/work in progress labels Oct 16, 2016

mujx suggested changes Oct 17, 2016

View reviewed changes

farodin91 force-pushed the tags branch 2 times, most recently from 152f276 to 4134d2c Compare October 17, 2016 20:02

mujx suggested changes Oct 17, 2016

View reviewed changes

farodin91 force-pushed the tags branch from 4134d2c to d7be231 Compare October 17, 2016 22:15

mujx approved these changes Oct 17, 2016

View reviewed changes

jimmycuadra reviewed Nov 24, 2016

View reviewed changes

src/api/r0/tags.rs

use tags::RoomTag;

use user::User;

pub type MapTags = BTreeMap<String, Value>;

Copy link

Member

jimmycuadra Nov 24, 2016

Choose a reason for hiding this comment

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

Why BTreeMap over HashMap?

farodin91 force-pushed the tags branch from d7be231 to bae4756 Compare November 26, 2016 17:43

farodin91 added the status/blocked label Dec 2, 2016

farodin91 added status/work in progress and removed status/needs review status/blocked labels Dec 2, 2016

farodin91 force-pushed the tags branch from bae4756 to 9e2ef66 Compare December 8, 2016 12:34

farodin91 added status/needs review and removed status/work in progress labels Dec 8, 2016

mujx suggested changes Dec 8, 2016

View reviewed changes

farodin91 force-pushed the tags branch from 9e2ef66 to 8380b30 Compare December 9, 2016 10:10

Add basic implementation for the tags endpoint.

8380b30

mujx approved these changes Dec 9, 2016

View reviewed changes

jimmycuadra merged commit fba8be9 into ruma:master Dec 21, 2016

farodin91 deleted the tags branch April 29, 2017 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic implementation for the tags endpoint. #105

Add basic implementation for the tags endpoint. #105

farodin91 commented Oct 12, 2016

farodin91 commented Oct 15, 2016

farodin91 commented Oct 15, 2016

farodin91 commented Oct 15, 2016

mujx Oct 17, 2016

farodin91 Oct 17, 2016

mujx Oct 17, 2016

farodin91 Oct 17, 2016

mujx Oct 17, 2016

farodin91 Oct 17, 2016

mujx Oct 17, 2016

farodin91 Oct 17, 2016

jimmycuadra Nov 24, 2016

farodin91 commented Dec 2, 2016

farodin91 commented Dec 8, 2016

mujx Dec 8, 2016

mujx Dec 8, 2016

farodin91 Dec 8, 2016

mujx Dec 8, 2016

mujx Dec 8, 2016

mujx Dec 8, 2016

mujx Dec 8, 2016

mujx Dec 8, 2016

farodin91 Dec 8, 2016

mujx Dec 8, 2016

mujx Dec 8, 2016

farodin91 Dec 9, 2016

farodin91 Dec 9, 2016

mujx Dec 8, 2016

jimmycuadra commented Dec 21, 2016

Add basic implementation for the tags endpoint. #105

Add basic implementation for the tags endpoint. #105

Conversation

farodin91 commented Oct 12, 2016

farodin91 commented Oct 15, 2016

farodin91 commented Oct 15, 2016

farodin91 commented Oct 15, 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

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

Choose a reason for hiding this comment

farodin91 commented Dec 2, 2016

farodin91 commented Dec 8, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmycuadra commented Dec 21, 2016