Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement API tagging location #2847

Merged
merged 3 commits into from
Jul 5, 2018
Merged

Implement API tagging location #2847

merged 3 commits into from
Jul 5, 2018

Conversation

stefannibrasil
Copy link
Contributor

Improve API location search (#1934 ) to also include tags on the request URL.

Fix #2790 and #2254

  • [ ✔️] tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • [✔️ ] code is in uniquely-named feature branch and has no merge conflicts
  • [✔️ ] PR is descriptively titled
  • [✔️ ] PR body includes fixes #0000-style reference to original issue #
  • [✔️ ] ask @publiclab/reviewers for help, in a comment below

@stefannibrasil
Copy link
Contributor Author

Hey, @Gauravano @sagarpreet-chadha @jywarren could you please take a look here? I've opened this PR to gather some feedback, what do you think? Thanks!

@stefannibrasil
Copy link
Contributor Author

stefannibrasil commented Jun 19, 2018

@publiclab/reviewers please review :)

I'll fix the codeclimate issues tomorrow.

@plotsbot
Copy link
Collaborator

plotsbot commented Jun 19, 2018

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.
📖 Your pull request is on the master branch. Please make a separate feature branch) with a descriptive name like new-blog-design while making PRs in the future.

Generated by 🚫 Danger

@jywarren
Copy link
Member

Looking great!!!

@sagarpreet-chadha what do you think of this?

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

@stefannibrasil ...the changes are great !

coordinates = srchString.split(",")
lat = coordinates[0]
lon = coordinates[1]
lat, lon = srchString.split(',')
Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing 😄 !

@sagarpreet-chadha
Copy link
Contributor

sagarpreet-chadha commented Jun 20, 2018

@stefannibrasil ...can you post a screenshot of the the JSON ?
By visiting here : http://localhost:3000/api/srch/taglocations?srchString=.........
Thanks !

@stefannibrasil
Copy link
Contributor Author

stefannibrasil commented Jun 20, 2018

Sure!

By calling api/srch/taglocations?srchString=49.00,123.00&tagName=testing we have:

imageedit_2_6877554811

Do you see anything else to improve here? Thanks for your feedback, everyone! :)

lon = coordinates[1]
lat, lon = srchString.split(',')

tagList = textSearch_tags(tagName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @stefannibrasil !
This looks great !
Just one thing --- should we not show nodes with tagName which our near the given Latitude and Longitude .
This will add all nodes having tag = tagName , right ?
What do you think ?

@sagarpreet-chadha
Copy link
Contributor

sagarpreet-chadha commented Jun 21, 2018

AND @stefannibrasil ...if a tagname is given then we need to show - nodes near given LAT and LON value having tag = tagname .

Else if , tagname is not given in URL then we need to show all near by nodes .

@jywarren ...can you confirm this ?

@jywarren
Copy link
Member

Yep, that's right!

@stefannibrasil
Copy link
Contributor Author

Right, I'll work on this, makes sense. Thanks, I'll let you know when I have it changed, cheers!

@jywarren
Copy link
Member

Everything going well here? Just checking, thanks!

@stefannibrasil
Copy link
Contributor Author

HI, @jywarren sorry, I am a little late with this one.

I will work on this tomorrow, I am getting some things done before RGSoC starts, that's why I couldn't finish this. Thanks for checking in! =)

@jywarren
Copy link
Member

jywarren commented Jun 29, 2018 via email

@stefannibrasil
Copy link
Contributor Author

hi, @jywarren @sagarpreet-chadha I made some changes, could you take a look to see if that's what we need, please?
Also, I am not really sure what is happening with this Junit parsing error, my tests are running okay here :/ Thanks!

sresult = DocList.new
unless params[:srchString].nil? || params[:srchString] == 0 || !(params[:srchString].include? ",")
sservice = SearchService.new
sresult = sservice.nearbyNodes(params[:srchString])
sresult = sservice.tagNearbyNodes(params[:srchString], params[:tag])
Copy link
Contributor

Choose a reason for hiding this comment

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

params[:tag] should be params[:tagName] , right ?


nids ||= []
if tagName.present?
nodes_scope = nodes_scope.where('name LIKE ?', tagName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome 💯 !

@ghost ghost assigned milaaraujo Jul 4, 2018
@milaaraujo
Copy link
Collaborator

Hello @jywarren @sagarpreet-chadha, Stefanni and I made more changes. When possible, could you take a look? Thanks!

@jywarren jywarren merged commit dbc455f into publiclab:master Jul 5, 2018
@ghost ghost removed the review-me label Jul 5, 2018
@jywarren
Copy link
Member

jywarren commented Jul 5, 2018

This looks awesome and I love the tests. Great work!!! 🎉 👍 👍

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* API tagging locations implementation

* Requested changes for the API tagging implementation

* Requested changes for the API tagging implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API tagging implementation
5 participants