Remove API Search endpoints duplication #3045

stefannibrasil · 2018-07-11T04:28:14Z

Hello, everyone!

We are opening this PR to get some feedback about our work with the API.

In order to refactor how the searches are being made, we decided to first try to remove some duplications. We have done some small changes but we wanted to know what do you think and see if there is another thing we can remove/change.

@publiclab/reviewers @publiclab/soc @jywarren

jywarren · 2018-07-11T19:27:10Z

Hi, this is due to a new check that was added recently to encourage very consistent formatting. I've actually turned off the check until we can make it's output friendlier, but since your work is based on an earlier version of code, you're seeing some formatting suggestions. You can resolve them by going through them one by one here:

https://travis-ci.org/publiclab/plots2/builds/402502945#L3400

Sorry, it's a bit obscure, we're working to make this more readable!!! Thanks!

plotsbot · 2018-07-12T03:50:53Z

	2 Messages
📖	@stefannibrasil Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖	This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: `Fixes #123`.

Generated by 🚫 Danger

jywarren · 2018-07-12T17:52:21Z

Cool! got the tests to pass, i see! Now, CodeClimate is complaining about "cognitive complexity" -- basically, asking you to try to simplify how the code is written in order to make it more readable.

CodeClimate is picky -- any time you think it's being too particular, you can ping me and I can manually "approve" a PR and we can move forward -- not a problem! But if you click "details" next to CodeClimate, it'll offer some advice: https://codeclimate.com/github/publiclab/plots2/pull/3045

If you hover over the right-side of each issue, it has a "read more" link which offers some tips on how to resolve it:

These are actually really nice and helpful despite the Xs!

For this one, I'm OK with how you've done it. If you'd like, I can just approve it -- just tell me what you think!

Actually, what I just wrote in this comment is a nice thing we could add to a README in the repository... what do you think? I'd accept a PR with this text in a CODECLIMATE.md file... if you're interested! We could point new contributors toward it to help them deal with CodeClimate errors.

Thanks!

jywarren · 2018-07-12T17:52:48Z

@mridulnagpal is working on CodeClimate on his own project and may find this interesting!

stefannibrasil · 2018-07-13T01:58:18Z

HI, Jeff!

Yeah, sorry, I forgot to tell that we fixed the small issues. I didn't say anything because we are still working on this. We are breaking the responsibilities into new classes so we may fix that issue. Just having a little problem trying to understand a Grape error for now! :P

And great idea about the codeclimate.md file! We can start that for sure :)

milaaraujo · 2018-07-13T05:04:14Z

Hey @jywarren, today we worked on our code to remove cognitive complexity from the Search API. We still have to work on minor repairs, but we'll do it tomorrow!

milaaraujo · 2018-07-13T21:34:09Z

Hey @jywarren, it looks like all checks have passed now!

stefannibrasil · 2018-07-14T02:25:12Z

@publiclab/soc @publiclab/reviewers hey, everyone, just one question regarding the Typeahead Service.
We did the same thing as this PR does to the Search Service but we are not really sure what this does in the Typeahead code:

present sresult, with: TagList::Entity

We run the tests and searched locally the API and we couldn't see any difference in the sresult list between using that or removing it. Can someone please explain more about this? Thank you!

jywarren · 2018-07-15T13:08:06Z

Hmm, I'm not sure about that either, but can try to trace through the code to figure it out. Can you find documentation for the present keyword? TagList is a class custom built for this codebase by @david-days - maybe he can chime in? Can you link directly to the line of code in question?

sagarpreet-chadha · 2018-07-16T16:42:07Z

Hi @stefannibrasil ...

plots2/app/models/tag_list.rb

Line 28 in 40a207b

class Entity < Grape::Entity

In comments above this internal-class is written :
This subclass is used to auto-generate the RESTful data structure. It is generally not useful for internal Ruby usage but must be included for full RESTful functionality.

Hope it helps !

milaaraujo · 2018-07-16T21:52:36Z

Hey @jywarren, @stefannibrasil's question (present keyword) is not directly connect with this PR (#3045). So are still waiting you to approve this one! 😬 And then we can open a new PR for removing API Typeahead endpoints duplication.

stefannibrasil · 2018-07-17T01:35:37Z

@milaaraujo is right! We are waiting for the review of this PR before opening the Typeahead refactor proposal. We did the same thing as we did here with the Search.

While this one isn't approved, I decided to ask about the Typeahead service. This is the line that I was referring to (we can continue the discussion after this current PR is reviewed/approved):

plots2/app/api/srch/typeahead.rb

Line 27 in 40a207b

present sresult, with: TagList::Entity

We wanted to understand why Typeahead adds that to each endpoint call. If we remove it, we don't see any difference in the results format, so we don't understand what exactly that line is supposed to do.

@sagarpreet-chadha do you have an idea why only typeahead use that? the search service doesn't use the DocList::Entity 🤔

And even if we remove that line, we still get the results as a list:

{
  "items": [
    {
      "tagId": 8,
      "tagVal": "Blog Post",
      "tagType": "file",
      "tagSource": "/notes/admin/07-10-2018/blog-post"
    }
  ],
  "srchParams": {
    "srchString": "Blog",
    "seq": null,
    "showCount": null,
    "pageNum": null,
    "tagName": null
  }
}

so there is no apparent difference between using that or not. The result is already a TagList. I have a guess that we don't need that at all, but we wanted to know what do you think 💭

jywarren

This is awesome, i love it. Great work on breaking into separate files. I asked for a couple extra comments to help orient people who aren't familiar with how the files are split up. Then I think we're good!

jywarren · 2018-07-17T15:13:55Z

test/functional/search_api_test.rb

@@ -166,29 +211,4 @@ def app

  end

-  test 'search recent people functionality having specified tagName' do


Did you remove this because it's covered by the all test? Just checking!

jywarren · 2018-07-17T15:15:47Z

app/api/srch/search.rb

@@ -3,6 +3,8 @@

 module Srch
  class Search < Grape::API
+    helpers SharedParams


Should we write a comment like # see /app/api/srch/shared_params.rb ?

jywarren · 2018-07-17T15:16:34Z

app/api/srch/shared_params.rb

+    extend Grape::API::Helpers
+
+    params :common do
+      requires :srchString, type: String, documentation: { example: 'Spec' }


This is fantastic. Thanks!!!

@jywarren we added the test back. We'll probably work more on the tests in the next weeks, but just to be more careful, we think it's best to not remove it for now. Good catch!

we've already done the same strategy here in the Typeahead, we are planning to open a PR for that after this is approved!

Thanks! 💯

milaaraujo · 2018-07-18T06:16:41Z

Hi guys, I'm adding a last modification - I hope so! - to the code. recentPeople method should "GET X number of latest people/contributors, X = srchString". So I changed from:

nodes = Node.all.order("changed DESC").limit(100).distinct -> nodes = Node.all.order("changed DESC").limit(srchString).distinct

jywarren · 2018-07-18T13:46:56Z

I think the error we're seeing may have been resolved in another PR - so you should be able to rebase your work over the latest master and it should disappear. However I've restarted the build to see if it "resolves itself" first... you can always re-run Travis tests by closing and reopening the PR.

You can rebase by following the process outlined here, but you may be able to skip step 2 if your master is still in sync with publiclab's master: https://publiclab.org/wiki/contributing-to-public-lab-software#Rewinding+the+master+branch

Thanks!

milaaraujo · 2018-07-18T14:20:31Z

It passed now, @jywarren!

jywarren · 2018-07-18T14:32:51Z

Hooray!!!! 🎉 🎉 🎉

* WIP - Remove API Search enpoints duplication * Remove cognitive complexity from Search api * Changes requested and clodeclimate fixes * Adding limit=srchString to recentPeople method

ghost assigned stefannibrasil Jul 11, 2018

ghost added the in progress label Jul 11, 2018

stefannibrasil assigned milaaraujo Jul 11, 2018

WIP - Remove API Search enpoints duplication

8dc4729

stefannibrasil force-pushed the refactor-api branch from 2b32937 to 8dc4729 Compare July 12, 2018 03:40

stefannibrasil changed the title ~~WIP - Remove API Search enpoints duplication~~ Remove API Search enpoints duplication Jul 14, 2018

stefannibrasil changed the title ~~Remove API Search enpoints duplication~~ Remove API Search endpoints duplication Jul 15, 2018

stefannibrasil mentioned this pull request Jul 15, 2018

Refactor Search API #3070

Closed

3 tasks

stefannibrasil mentioned this pull request Jul 17, 2018

Community Check-in #9 Summer Of Code Week 10 #3078

Closed

jywarren requested changes Jul 17, 2018

View reviewed changes

Remove cognitive complexity from Search api

b5ed69d

stefannibrasil force-pushed the refactor-api branch 2 times, most recently from 32bb9cd to ac12304 Compare July 18, 2018 01:47

Changes requested and clodeclimate fixes

5b8bcb7

stefannibrasil force-pushed the refactor-api branch from ac12304 to 5b8bcb7 Compare July 18, 2018 01:51

Adding limit=srchString to recentPeople method

a671849

jywarren merged commit 5c94fe5 into publiclab:master Jul 18, 2018

ghost removed the in progress label Jul 18, 2018

milaaraujo mentioned this pull request Jul 19, 2018

Remove API Typeahed endpoints duplication #3096

Closed

5 tasks

stefannibrasil deleted the refactor-api branch July 21, 2018 03:40

This was referenced Jul 22, 2018

Search endpoint most recently people #3126

Closed

Improve textSearch_profiles result #3134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove API Search endpoints duplication #3045

Remove API Search endpoints duplication #3045

stefannibrasil commented Jul 11, 2018 •

edited

Loading

jywarren commented Jul 11, 2018

plotsbot commented Jul 12, 2018 •

edited

Loading

jywarren commented Jul 12, 2018

jywarren commented Jul 12, 2018

stefannibrasil commented Jul 13, 2018

milaaraujo commented Jul 13, 2018

milaaraujo commented Jul 13, 2018

stefannibrasil commented Jul 14, 2018

jywarren commented Jul 15, 2018 •

edited

Loading

sagarpreet-chadha commented Jul 16, 2018

milaaraujo commented Jul 16, 2018

stefannibrasil commented Jul 17, 2018 •

edited

Loading

jywarren left a comment

jywarren Jul 17, 2018

jywarren Jul 17, 2018

jywarren Jul 17, 2018

stefannibrasil Jul 18, 2018 •

edited

Loading

milaaraujo commented Jul 18, 2018

jywarren commented Jul 18, 2018 •

edited

Loading

milaaraujo commented Jul 18, 2018

jywarren commented Jul 18, 2018

		@@ -166,29 +211,4 @@ def app

		end

		test 'search recent people functionality having specified tagName' do

Remove API Search endpoints duplication #3045

Remove API Search endpoints duplication #3045

Conversation

stefannibrasil commented Jul 11, 2018 • edited Loading

jywarren commented Jul 11, 2018

plotsbot commented Jul 12, 2018 • edited Loading

jywarren commented Jul 12, 2018

jywarren commented Jul 12, 2018

stefannibrasil commented Jul 13, 2018

milaaraujo commented Jul 13, 2018

milaaraujo commented Jul 13, 2018

stefannibrasil commented Jul 14, 2018

jywarren commented Jul 15, 2018 • edited Loading

sagarpreet-chadha commented Jul 16, 2018

milaaraujo commented Jul 16, 2018

stefannibrasil commented Jul 17, 2018 • edited Loading

jywarren left a comment

Choose a reason for hiding this comment

jywarren Jul 17, 2018

Choose a reason for hiding this comment

jywarren Jul 17, 2018

Choose a reason for hiding this comment

jywarren Jul 17, 2018

Choose a reason for hiding this comment

stefannibrasil Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

milaaraujo commented Jul 18, 2018

jywarren commented Jul 18, 2018 • edited Loading

milaaraujo commented Jul 18, 2018

jywarren commented Jul 18, 2018

stefannibrasil commented Jul 11, 2018 •

edited

Loading

plotsbot commented Jul 12, 2018 •

edited

Loading

jywarren commented Jul 15, 2018 •

edited

Loading

stefannibrasil commented Jul 17, 2018 •

edited

Loading

stefannibrasil Jul 18, 2018 •

edited

Loading

jywarren commented Jul 18, 2018 •

edited

Loading