API: Fix tags update on updating a note #2649

jyuvaraj03 · 2020-03-03T18:58:52Z

Fixes #941

Before:

After:

The issue as mentioned in #941 was that the tags were not updated. This was due to the fact that in PUT request method the tags were not explicitly set unlike how the POST request method was handled in the action_notes function.

The fix I have given makes the necessary changes. I have also written unit tests for the code. Looking forward to any comments or suggestions!

jyuvaraj03 · 2020-03-03T19:00:12Z

@PackElend label me please

miciasto · 2020-03-03T22:30:28Z

ReactNativeClient/lib/services/rest/Api.js

+
+			if (!note) throw new ErrorNotFound();
+
+			let newNote = Object.assign({}, note, request.bodyJson(this.readonlyProperties('PUT')));


I'm not familiar with this interface so please help me understand. Is the PUT command intended to update an existing note? Is this really a new note, or an updated one?

Yes, the PUT command is intended to update an existing note. Perhaps, the variable name newNote was a bit confusing? I will change it to updatedNote.

miciasto · 2020-03-03T22:31:48Z

ReactNativeClient/lib/services/rest/Api.js

+			if (!note) throw new ErrorNotFound();
+
+			let newNote = Object.assign({}, note, request.bodyJson(this.readonlyProperties('PUT')));
+			newNote = await Note.save(newNote, { userSideValidation: true });


What is the effect of user-side validation here?

The code I have written here was actually refactored from the defaultAction_ function which was previously called in the case of PUT methods.

To be honest, I do not exactly know what user-side validation does here. But since it was already present in the code that was executed (before this fix), I decided to leave it untampered in order not to break any functionality.

miciasto · 2020-03-03T22:35:58Z

CliClient/tests/services_rest_Api.js

+		const tag3 = await Tag.save({ title: 'mon étiquette 3' });
+		const note = await Note.save({
+			title: 'ma note un',
+			tags: `${tag1.title},${tag2.title}`,


Does this add the tags correclty? Perhaps in the test you can start checking if the note is set up as you intended.

Yes, it does add the tags in the note (checked in the desktop app). But I haven't checked the note in the unit test. I will rewrite the unit test to make sure it tests the notes rather than the response.

Since you have the response checks, they are still useful. So just add the new checks, so it checks both.

miciasto · 2020-03-03T22:42:01Z

CliClient/tests/services_rest_Api.js

+			tags: `${tag1.title},${tag2.title},${tag3.title}`,
+		}));
+		expect(response3.tags === `${tag1.title},${tag2.title},${tag3.title}`).toBe(true);
+	}));
 });


What happens if the user accidently sets a tag name that doesn't exist? Is it possible to do via this interface? If it is, then we should add a test for it.

If the user sets a tag name that doesn't exist via this interface, it creates the tags and then adds it to the note. I forgot to write a test case for that. I will fix the unit test.

miciasto

With this testing, you are checking the API response to confirm if the tags are set. In the issue, it looks like the responses were OK, but the actual database was not updated.

Should you also check the actual tags on the note note after each PUT (using Tag and/or Note methods to directly interrogate the associated tags)? I'm not sure the test is covering the issue.

jyuvaraj03 · 2020-03-04T14:09:08Z

Should you also check the actual tags on the note note after each PUT (using Tag and/or Note methods to directly interrogate the associated tags)? I'm not sure the test is covering the issue.

Yes, you were right. I should have checked the actual tags on the notes themselves. I have since rewritten the unit tests to do this.

jyuvaraj03 · 2020-03-05T12:51:29Z

I have done all the requested changes. Please review :)

miciasto

Thanks @jyuvaraj03, just a few more comments, all minor except one.

miciasto · 2020-03-06T05:52:16Z

CliClient/tests/services_rest_Api.js

+		expect(tagIds1[0]).toBe(tag1.id);
+		expect(tagIds1[1]).toBe(tag3.id);


Although this might work, there is no guarantee the tags will be returned in this order. Please modify so it is not dependent on the order.

miciasto · 2020-03-06T06:11:19Z

ReactNativeClient/lib/services/rest/Api.js

@@ -451,6 +451,23 @@ class Api {
 			return note;
 		}

+		if (request.method === 'PUT') {
+			const note = await Note.load(id);
+			const requestBody = JSON.parse(request.body);


For consistency we should change the name of 'requestBody' to match what is used in the 'POST' code (just above).

Also there is no need to create this so early in the function, so please move it closer to where it is first used.

Also there is no need to create this so early in the function, so please move it closer to where it is first used.

I have made this change. But, can I know why it is important to not create it early in the function?

miciasto · 2020-03-06T06:32:20Z

ReactNativeClient/lib/services/rest/Api.js

+
+			if (requestBody.tags) {
+				const tagTitles = requestBody.tags.split(',');
+				await Tag.setNoteTagsByTitles(updatedNote.id, tagTitles);


updatedNote.id can be just id

miciasto · 2020-03-06T06:33:08Z

ReactNativeClient/lib/services/rest/Api.js

+			let updatedNote = Object.assign({}, note, request.bodyJson(this.readonlyProperties('PUT')));
+			updatedNote = await Note.save(updatedNote, { userSideValidation: true });


You mentioned you borrowed this from the default handler. So rather than duplicate the code here, why not call the default handler to do this?

Yeah, I didn't think about that! I'll implement it now.

jyuvaraj03 · 2020-03-06T11:54:36Z

Completed all the requested changes. Please review!

laurent22 · 2020-03-06T23:23:10Z

@mic704b, I've sent you an invite to be a collaborator on the project, so when you think this one is ready feel free to merge it.

Please use the syntax Platform: Fixes|Resolves #xxx: Comment. The comment will appear in the changelog so it should be something not too technical (indeed the title of this PR would be good). So for example Api: Resolves #941: Fix tags update on updating a note

jyuvaraj03 · 2020-03-08T10:34:20Z

Sorry, I accidentally requested a review! Please review at your own pace.

miciasto · 2020-03-08T11:44:59Z

@mic704b, I've sent you an invite to be a collaborator on the project, so when you think this one is ready feel free to merge it.

Thank you @laurent22! I am honoured to be invited.

CliClient/tests/services_rest_Api.js

miciasto · 2020-03-08T12:11:57Z

@jyuvaraj03 looks good, nearly there.

jyuvaraj03 · 2020-03-09T18:03:37Z

Please add a test case for when tags are an empty string. It is not expected, but we could receive it, so we should test we handle it properly.

Thanks for pointing it out. That was an excellent catch! I hadn't handled that case. When the tags in the request is empty, the tags associated with the note do not get deleted like intended. Instead, they stay the same.

So I handled it by checking if the tags property is explicitly given as '' during the API request. If so, all the existing tags are removed from the corresponding note. If the tags property is not mentioned during the request, no change is made to the tags in the note.

jyuvaraj03 · 2020-03-09T18:11:38Z

CliClient/tests/services_rest_Api.js

+		const response4 = await api.route('PUT', `notes/${note.id}`, null, JSON.stringify({
+			tags: '',
+		}));
+		const tagIds4 = await NoteTag.tagIdsByNoteId(note.id);
+		expect(response4.tags === '').toBe(true);
+		expect(tagIds4.length === 0).toBe(true);
+	}));


In this test case, the tags property is mentioned explicitly with a ''. So, all the tags associated with the note are removed from the note.

jyuvaraj03 · 2020-03-09T18:11:41Z

CliClient/tests/services_rest_Api.js

+		const response3 = await api.route('PUT', `notes/${note.id}`, null, JSON.stringify({
+			title: 'Some other title',
+		}));
+		const tagIds3 = await NoteTag.tagIdsByNoteId(note.id);
+		expect(tagIds3.length === 2).toBe(true);
+		expect(tagIds3.includes(tag2.id)).toBe(true);
+		expect(tagIds3.includes(newTag.id)).toBe(true);


This test case checks the case when the tags property is not mentioned during the request. So, the tags associated with the note does not differ from the previous test case.

Also, I haven't checked response3.tags because it is undefined when tags property isn't mentioned during the API request. So, I skipped that check.

because it is undefined when tags property isn't mentioned

That can be tested too! If it wasn't returned as undefined, then it would be a problem.

miciasto

Looks good, a few more minor comments.

One more question: does the API documentation need update? The documentation says PUT on a note is to set note properties, but doesn't list tags as a property. Perhaps it should be added to the list, or the description changed to something like Sets the properties and tags of .... What do you think?

If it needs change, please do include it on this PR.

miciasto · 2020-03-11T11:18:31Z

ReactNativeClient/lib/services/rest/Api.js

+			if (requestNote.tags === '') {
+				await Tag.setNoteTagsByTitles(id, '');
+			} else if (requestNote.tags) {
+				const tagTitles = requestNote.tags.split(',');
+				await Tag.setNoteTagsByTitles(id, tagTitles);
+			}


Can this be reduced to a single if statement? I don't think you need two branches.

Yes, I just realized two branches weren't necessary. I changed the code to use just a single if.

miciasto · 2020-03-11T11:23:17Z

CliClient/tests/services_rest_Api.js

+		const tagIds4 = await NoteTag.tagIdsByNoteId(note.id);
+		expect(response4.tags === '').toBe(true);
+		expect(tagIds4.length === 0).toBe(true);
+	}));


Generally, for future maintenance, it would be better to separate the test cases into 3 or 4 smaller test cases rather than one big one. This also helps avoid using the 1,2,3,4 variable names too. Sometimes it is too inefficient, but by default it is preferred.

I restructured all the test cases into separate ones. But, I'm not sure if I did it right. I have used lengthy test case descriptions for some test cases. Please review and let me know if I'm doing it right.

Looks good!

miciasto · 2020-03-11T11:31:12Z

CliClient/tests/services_rest_Api.js

+		const response3 = await api.route('PUT', `notes/${note.id}`, null, JSON.stringify({
+			title: 'Some other title',
+		}));
+		const tagIds3 = await NoteTag.tagIdsByNoteId(note.id);
+		expect(tagIds3.length === 2).toBe(true);
+		expect(tagIds3.includes(tag2.id)).toBe(true);
+		expect(tagIds3.includes(newTag.id)).toBe(true);


because it is undefined when tags property isn't mentioned

That can be tested too! If it wasn't returned as undefined, then it would be a problem.

laurent22 · 2020-03-11T14:16:27Z

One more question: does the API documentation need update? The documentation says PUT on a note is to set note properties, but doesn't list tags as a property. Perhaps it should be added to the list, or the description changed to something like Sets the properties and tags of .... What do you think?

That's a good point, the doc should be updated. To do so @jyuvaraj03 you can update command-apidoc.js, then you can verify the output from /CliClient using npm start -- apidoc.

In fact, ideally we should support all the special properties that the POST method supports, such as body_html, image_data_url, crop_rect, etc. for consistency. That could be done with a bit of refactoring, but it's not needed for this PR.

jyuvaraj03 · 2020-03-12T16:18:14Z

One more question: does the API documentation need update? The documentation says PUT on a note is to set note properties, but doesn't list tags as a property. Perhaps it should be added to the list, or the description changed to something like Sets the properties and tags of .... What do you think?

You're right. So, I updated the doc. I added the tags property to the list of properties.

jyuvaraj03 · 2020-03-12T16:27:31Z

That's a good point, the doc should be updated. To do so @jyuvaraj03 you can update command-apidoc.js, then you can verify the output from /CliClient using npm start -- apidoc.

Yes, I did update the command-apidoc.js file. But, I have a small question. These were the lines of code I changed in that file:

tableFields.push({
	name: 'tags',
	type: Database.enumId('fieldType', 'text'),
	description: 'Comma-separated list of tags. eg. `tag1,tag2`.',
});

I just uncommented them in this commit. These lines already existed, but you had commented them out. Was there a reason you did that? Am I missing something?

In fact, ideally we should support all the special properties that the POST method supports, such as body_html, image_data_url, crop_rect, etc. for consistency. That could be done with a bit of refactoring, but it's not needed for this PR.

I would like to work on implementing these (possibly in another PR). How should I go about it? Should I create an issue for this and then submit a PR?

Also, @PackElend can you please @mention Laurent here because he was the one who commented those lines out, and I would like to know why he did that.

laurent22 · 2020-03-13T18:44:15Z

Ok I think it's ready to merge now, thanks @jyuvaraj03 and thanks @mic704b for reviewing!

miciasto · 2020-03-13T20:59:10Z

I think it's ready to merge now

Definitely.

Good work @jyuvaraj03

jyuvaraj03 added 2 commits March 3, 2020 23:26

API: Fix updation of tags when updating a note

df04e99

Add test for fix updation of tags

81582c6

PackElend added the gsoc-2020 label Mar 3, 2020

miciasto reviewed Mar 3, 2020

View reviewed changes

miciasto reviewed Mar 4, 2020

View reviewed changes

Rewrite unit test and change variable name

1ad0e29

tessus added the api Joplin API label Mar 6, 2020

miciasto suggested changes Mar 6, 2020

View reviewed changes

Use consistent variables and function calls

cf57fbf

Use default function call and change variable

023be8b

jyuvaraj03 requested a review from miciasto March 8, 2020 09:08

miciasto suggested changes Mar 8, 2020

View reviewed changes

CliClient/tests/services_rest_Api.js Show resolved Hide resolved

CliClient/tests/services_rest_Api.js Outdated Show resolved Hide resolved

Handle case when tags is empty string

f789567

jyuvaraj03 commented Mar 9, 2020

View reviewed changes

miciasto reviewed Mar 11, 2020

View reviewed changes

jyuvaraj03 added 2 commits March 12, 2020 19:45

Restructure test cases and improve tags check

644b132

Update documentation

cf19418

laurent22 merged commit cda8372 into laurent22:master Mar 13, 2020

jyuvaraj03 mentioned this pull request Apr 13, 2020

API: PUT method doesn't support all properties that POST method supports for notes #3046

Closed


		if (!note) throw new ErrorNotFound();

		let newNote = Object.assign({}, note, request.bodyJson(this.readonlyProperties('PUT')));

		expect(tagIds1[0]).toBe(tag1.id);
		expect(tagIds1[1]).toBe(tag3.id);

		let updatedNote = Object.assign({}, note, request.bodyJson(this.readonlyProperties('PUT')));
		updatedNote = await Note.save(updatedNote, { userSideValidation: true });

Uh oh!

API: Fix tags update on updating a note #2649

API: Fix tags update on updating a note #2649

Uh oh!

Conversation

jyuvaraj03 commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyuvaraj03 commented Mar 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyuvaraj03 Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miciasto Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miciasto Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miciasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyuvaraj03 commented Mar 4, 2020

Uh oh!

jyuvaraj03 commented Mar 5, 2020

Uh oh!

miciasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyuvaraj03 commented Mar 6, 2020

Uh oh!

laurent22 commented Mar 6, 2020

Uh oh!

jyuvaraj03 commented Mar 8, 2020

Uh oh!

miciasto commented Mar 8, 2020

Uh oh!

Uh oh!

Uh oh!

miciasto commented Mar 8, 2020

Uh oh!

jyuvaraj03 commented Mar 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miciasto left a comment

Choose a reason for hiding this comment

Uh oh!

jyuvaraj03 commented Mar 3, 2020 •

edited

Loading

jyuvaraj03 Mar 4, 2020 •

edited

Loading

miciasto Mar 3, 2020 •

edited

Loading

miciasto Mar 3, 2020 •

edited

Loading

miciasto left a comment •

edited

Loading

jyuvaraj03 commented Mar 12, 2020 •

edited

Loading