/ruma

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add basic implementation for the tags endpoint. #105

Merged
merged 1 commit into from Dec 21, 2016

Conversation

Reviewers

@jimmycuadra jimmycuadra

@mujx mujx

Assignees
No one assigned
Labels
component/api spec/tags status/needs review type/feature
Projects
None yet
Milestone
No milestone
3 participants
@farodin91 @jimmycuadra @mujx
@farodin91
Member

farodin91 commented Oct 12, 2016

No description provided.

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

This was referenced Oct 12, 2016

Closed
Closed
Closed

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

@farodin91
Member

farodin91 commented Oct 15, 2016

@jimmycuadra Review?
@farodin91
Member

farodin91 commented Oct 15, 2016

I need to add an event m.tag.
@farodin91
Member

farodin91 commented Oct 15, 2016

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

@farodin91 farodin91 closed this Oct 15, 2016

@farodin91 farodin91 reopened this Oct 15, 2016

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

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))?;
@mujx

mujx Oct 17, 2016

Member

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

@farodin91

farodin91 Oct 17, 2016

Member

Resolved

src/tags.rs
+ room_id: RoomId,
+ tag: String,
+ content: Value)
+ -> Result<(), ApiError> {
@mujx

mujx Oct 17, 2016

Member

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

@farodin91

farodin91 Oct 17, 2016

Member

Resolved

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") {
@mujx

mujx Oct 17, 2016

Member

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

@farodin91

farodin91 Oct 17, 2016

Member

Resolved!

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))?;
@mujx

mujx Oct 17, 2016

Member

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

@farodin91

farodin91 Oct 17, 2016

Member

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

@mujx

mujx approved these changes Oct 17, 2016
View changes

src/api/r0/tags.rs
+use tags::RoomTag;
+use user::User;
+
+pub type MapTags = BTreeMap<String, Value>;
@jimmycuadra

jimmycuadra Nov 24, 2016

Owner

Why BTreeMap over HashMap?

@farodin91 farodin91 added the status/blocked label Dec 2, 2016

@farodin91
Member

farodin91 commented Dec 2, 2016

Waiting for #114

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

@farodin91
Member

farodin91 commented Dec 8, 2016

@jimmycuadra Could you start reviewing? Thanks
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>()
@mujx

mujx Dec 8, 2016

Member

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.

@farodin91

farodin91 Dec 9, 2016

Member

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

@farodin91

farodin91 Dec 9, 2016

Member

Solved for all three, but not nice for delete.

src/api/r0/tags.rs
+ #[test]
+ fn basic_create_tag() {
+ let test = Test::new();
+ let access_token = test.create_access_token(); // @carl:ruma.test
@mujx

mujx Dec 8, 2016

Member

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

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",
@mujx

mujx Dec 8, 2016

Member

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.

@farodin91

farodin91 Dec 8, 2016

Member

I would do this after is fixed #121.

src/api/r0/tags.rs
+ }
+
+ #[test]
+ fn delete_tag_forbidden() {
@mujx

mujx Dec 8, 2016

Member

Why is this forbidden?

src/api/r0/tags.rs
+ }
+
+ #[test]
+ fn update_tag() {
@mujx

mujx Dec 8, 2016

Member

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

@farodin91

farodin91 Dec 8, 2016

Member

No, See Line 177, 179

@mujx

mujx Dec 8, 2016

Member

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

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),
@mujx

mujx Dec 8, 2016

Member

You can use the err instead of None.

src/models/tags.rs
+ map.insert(tag.tag, info);
+ }
+
+ let tags = TagEventContent { tags: map };
@mujx

mujx Dec 8, 2016

Member

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

src/models/tags.rs
+ user_id: UserId,
+ room_id: RoomId,
+ tag: String,
+ content: Value,
@mujx

mujx Dec 8, 2016

Member

Why Value instead of String?

src/models/tags.rs
+ delete(tag)
+ .execute(connection)
+ .map_err(|err| match err {
+ DieselError::NotFound => ApiError::not_found(None),
@mujx

mujx Dec 8, 2016

Member

Also a missing error message here.

@mujx

mujx approved these changes Dec 9, 2016
View changes

@jimmycuadra jimmycuadra merged commit fba8be9 into ruma:master Dec 21, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jimmycuadra
Owner

jimmycuadra commented Dec 21, 2016

Fantastic work. :D

@farodin91 farodin91 deleted the farodin91:tags branch Apr 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment