Add the new fullTextSearch method with the associated tests. #470

SebC99 · 2017-08-02T07:23:33Z

To be used with parse-server version > 2.5.0

codecov · 2017-08-02T07:30:24Z

Codecov Report

Merging #470 into master will increase coverage by 1.39%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
+ Coverage   84.03%   85.42%   +1.39%     
==========================================
  Files          47       46       -1     
  Lines        3801     3747      -54     
  Branches      869      859      -10     
==========================================
+ Hits         3194     3201       +7     
+ Misses        607      546      -61

Impacted Files	Coverage Δ
src/ParseQuery.js	`94.27% <88.23%> (+0.19%)`	⬆️
src/Parse.js	`81.53% <0%> (-0.28%)`	⬇️
src/encode.js	`100% <0%> (ø)`	⬆️
src/ParsePolygon.js
src/StorageController.react-native.js	`85.71% <0%> (+2.38%)`	⬆️
src/decode.js	`100% <0%> (+3.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f713a2c...1012bb0. Read the comment docs.

russell-dot-js

Overall it looks good, but there is some logic in fullTextSearch that either is buggy (if statement that always evaluates to true) or is hard to reason about and warrants test coverage

russell-dot-js · 2017-10-08T04:37:08Z

src/ParseQuery.js

+  fullTextSearch(key: string, search: string, language: string, caseSensitive: boolean, diacriticSensitive: boolean): ParseQuery {
+    if (typeof key === 'undefined' || !key) {
+      throw new Error('A key is required.');
+    }


redundant, could be simplified to if(!key)

russell-dot-js · 2017-10-08T04:37:23Z

src/ParseQuery.js

+      throw new Error('You have to add one string to search.');
+    }
+    var options = { '$term': search };
+    if (typeof language !== "undefined" || language !== null) {


inconsistent usage of single/double quoted strings

when is this statement false?

var a = null; typeof a !== 'undefined' // true var a = undefined; a !== null // true

russell-dot-js · 2017-10-08T04:39:38Z

src/ParseQuery.js

+    if (typeof caseSensitive !== "undefined" || caseSensitive !== null) {
+      options['$caseSensitive'] = caseSensitive;
+    }
+    if (typeof diacriticSensitive !== "undefined" || diacriticSensitive !== null) {


I'm struggling to see when this would ever be falsy

You're right, it was supposed to be a &&.
To be more precise we could go with something like:

if (typeof diacriticSensitive === "boolean") { options['$diacriticSensitive'] = diacriticSensitive; }

I prefer it to just test if (diacritic) to let user specifies a false value if he wants to for any reason.

montymxb · 2017-11-03T06:13:35Z

@SebC99 just wanted to check in and see if you're still working on this. For some of the notes above you can simplify them down just !param (like for requiring search and key). As for the rest, since they should be booleans, if(param) would do just fine.

SebC99 · 2017-11-03T08:17:29Z

@montymxb sorry I didn't see @russell-dot-js's comments! I'll check that asap

montymxb · 2017-11-03T22:59:52Z

src/ParseQuery.js

+   if (typeof language === 'string') {
+     options['$language'] = language;
+   }
+   if (typeof caseSensitive === "boolean") {


Should wrap "boolean" with single quotes like the ones above.

montymxb · 2017-11-03T23:00:00Z

src/ParseQuery.js

+   if (typeof caseSensitive === "boolean") {
+     options['$caseSensitive'] = caseSensitive;
+   }
+   if (typeof diacriticSensitive === "boolean") {


Same thing here as above

montymxb

Just some nits with the choice of single/double quotes.

Also can you add integration tests for this? They would go in here. Ideally we'll need a test case demonstrating functional use of each parameter and anti-cases if possible. Just paired cases where one indicates you can find something with fullText and given parameters, and another case where you do not expect to get an object.

As far as the current failure I'll look at this, but it seems unrelated to what you're presenting here.

Other than nice job on putting this together 👍 . Looking forward to checking out the changes!

montymxb · 2017-12-08T00:30:00Z

@SebC99 bump?

SebC99 · 2017-12-08T06:43:33Z

Sorry haven't got the time yet (and not very good at writing tests)!

montymxb · 2017-12-09T00:13:56Z

Gotcha. Well if you need an idea of what tests would look like you can checkout one of the test files for parse queries. Additionally that example file is probably where you would put your tests as well. Adding some new its would be what we're looking for. If you have any questions as to how to go about this let me know and I'll see if you I can dig up some links to get you in the right direction.

srameshr · 2018-01-08T09:49:11Z

Shit! This did not make it into the JS sdk?
Waiting for this! The current test search on Parse Server is barely useful.
@montymxb

SebC99 · 2018-01-09T10:46:23Z

@srameshr sorry I really don't have the time to add the integration tests for now :(

srameshr · 2018-01-09T11:00:53Z

@montymxb Any chance that you would make the changes and merge it in? I will update the docs.

dplewis · 2018-01-09T15:09:25Z

@srameshr I wanted to give @SebC99 a chance to get this in. I have some time later to add Full Text Search to this SDK. That reminds me to add it to the Android SDK as well.

Also the JS Docs will be auto generated

srameshr · 2018-01-09T17:29:56Z

@dplewis It would be great if you could do that. As of now I am manually copying and pasting that chuck of code.

montymxb · 2018-01-09T17:45:57Z

Sorry guys, I've been a bit busy lately. I'll need to get back on taking a look at everything here around Thursday or so.

…

On Jan 9, 2018 07:09, "Diamond Lewis" ***@***.***> wrote: @srameshr <https://github.com/srameshr> I wanted to give @SebC99 <https://github.com/sebc99> a chance to get this in. I have some time later to add Full Text Search to this SDK. That reminds me to add it to the Android SDK as well. Also the JS Docs will be auto generated — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#470 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE1d4KMTr_Bw4S2bt017b0u8dWXCRwe4ks5tI4EmgaJpZM4Oqsuy> .

srameshr · 2018-01-11T17:25:48Z

@montymxb @dplewis
Waiting for this to be merged!

dplewis · 2018-01-11T19:34:21Z

@srameshr I opened #541. It may take some time for approval and a new sdk release. You can always fork the repo, pull down the changes and use your own branch until the release.

montymxb · 2018-01-15T23:45:35Z

Closed in favor of #541 , still wanted to say thanks for taking the time to work on this @SebC99 .

SebC99 · 2018-01-16T07:30:52Z

Thanks a lot guys, and sorry for not having had the time to finish it!
💪

dplewis · 2018-01-16T13:44:27Z

@SebC99 No problem, thanks for getting started on this requested feature. 👍

dplewis mentioned this pull request Sep 27, 2017

Full Text Search #476

Closed

russell-dot-js suggested changes Oct 8, 2017

View reviewed changes

montymxb mentioned this pull request Nov 3, 2017

Full text search support parse-community/parse-server#3747

Closed

montymxb self-assigned this Nov 3, 2017

vinz51 and others added 2 commits November 3, 2017 11:02

Add the new fullTextSearch method with the associated tests.

7963861

Fix inadequate if statements

1012bb0

SebC99 force-pushed the feature/fulltextsearch branch from c29f31e to 1012bb0 Compare November 3, 2017 10:04

montymxb reviewed Nov 3, 2017

View reviewed changes

montymxb suggested changes Nov 3, 2017

View reviewed changes

srameshr mentioned this pull request Jan 8, 2018

Full text search for Parse JS SDK and or Parse Cloud code parse-community/parse-server#4490

Closed

dplewis mentioned this pull request Jan 10, 2018

Support for Full Text Search Query #541

Merged

montymxb closed this Jan 15, 2018

SebC99 deleted the feature/fulltextsearch branch August 27, 2022 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the new fullTextSearch method with the associated tests. #470

Add the new fullTextSearch method with the associated tests. #470

SebC99 commented Aug 2, 2017

codecov bot commented Aug 2, 2017 •

edited

Loading

russell-dot-js left a comment

russell-dot-js Oct 8, 2017

russell-dot-js Oct 8, 2017

russell-dot-js Oct 8, 2017 •

edited

Loading

russell-dot-js Oct 8, 2017

SebC99 Nov 3, 2017 •

edited

Loading

montymxb commented Nov 3, 2017

SebC99 commented Nov 3, 2017 •

edited

Loading

montymxb Nov 3, 2017

montymxb Nov 3, 2017

montymxb left a comment

montymxb commented Dec 8, 2017

SebC99 commented Dec 8, 2017

montymxb commented Dec 9, 2017

srameshr commented Jan 8, 2018 •

edited

Loading

SebC99 commented Jan 9, 2018

srameshr commented Jan 9, 2018

dplewis commented Jan 9, 2018

srameshr commented Jan 9, 2018

montymxb commented Jan 9, 2018 via email

srameshr commented Jan 11, 2018

dplewis commented Jan 11, 2018

montymxb commented Jan 15, 2018

SebC99 commented Jan 16, 2018

dplewis commented Jan 16, 2018

Add the new fullTextSearch method with the associated tests. #470

Add the new fullTextSearch method with the associated tests. #470

Conversation

SebC99 commented Aug 2, 2017

codecov bot commented Aug 2, 2017 • edited Loading

Codecov Report

russell-dot-js left a comment

Choose a reason for hiding this comment

russell-dot-js Oct 8, 2017

Choose a reason for hiding this comment

russell-dot-js Oct 8, 2017

Choose a reason for hiding this comment

russell-dot-js Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

russell-dot-js Oct 8, 2017

Choose a reason for hiding this comment

SebC99 Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

montymxb commented Nov 3, 2017

SebC99 commented Nov 3, 2017 • edited Loading

montymxb Nov 3, 2017

Choose a reason for hiding this comment

montymxb Nov 3, 2017

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

montymxb commented Dec 8, 2017

SebC99 commented Dec 8, 2017

montymxb commented Dec 9, 2017

srameshr commented Jan 8, 2018 • edited Loading

SebC99 commented Jan 9, 2018

srameshr commented Jan 9, 2018

dplewis commented Jan 9, 2018

srameshr commented Jan 9, 2018

montymxb commented Jan 9, 2018 via email

srameshr commented Jan 11, 2018

dplewis commented Jan 11, 2018

montymxb commented Jan 15, 2018

SebC99 commented Jan 16, 2018

dplewis commented Jan 16, 2018

codecov bot commented Aug 2, 2017 •

edited

Loading

russell-dot-js Oct 8, 2017 •

edited

Loading

SebC99 Nov 3, 2017 •

edited

Loading

SebC99 commented Nov 3, 2017 •

edited

Loading

srameshr commented Jan 8, 2018 •

edited

Loading