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

Update docs and add tests for the taglocations API method - fix #3597 #3700

Merged
merged 4 commits into from
Nov 13, 2018

Conversation

stefannibrasil
Copy link
Contributor

@stefannibrasil stefannibrasil commented Oct 15, 2018

Fixes #3597

The API docs were showing a wrong param format for the taglocations endpoint. After investigating, I believe these changes attempt to fix that.

@ghost ghost assigned stefannibrasil Oct 15, 2018
@ghost ghost added the in progress label Oct 15, 2018
@plotsbot
Copy link
Collaborator

plotsbot commented Oct 15, 2018

1 Message
📖 @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.

Generated by 🚫 Danger

@stefannibrasil
Copy link
Contributor Author

@publiclab/reviewers @jywarren is that ok pass this codeclimate issue for later? I just added more validations to the method... or if you have any tips for this, would appreciate them. Thanks!

@jywarren
Copy link
Member

Sure, passing it now! Thanks!

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

One small tweak but THANK YOU this is much better!!! 🎉

app/api/srch/search.rb Outdated Show resolved Hide resolved
* **URL Params** :

**Required:**

`query=[coordinates]`: search notes from users located near the query separated by `,`.
`query=[coordinates]`: search notes from users located near the query separated by `,`
Copy link
Member

Choose a reason for hiding this comment

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

👍

@jywarren
Copy link
Member

Also, sorry, I think you may have written this somewhere else but I'm a bit behind... is there a matching JS call which we have to tweak to conform to the .00 format?

@stefannibrasil
Copy link
Contributor Author

hi, @jywarren thanks for the review!

I don't know about the JS call, to be honest. Was that already been done somewhere? I'm not aware...

Removing blurred: false because it can be accessed via `user.blurred?`
@jywarren
Copy link
Member

Ah, here:

$.getJSON("/api/srch/taglocations?query="+s , function(data){

Do we need to pad it or otherwise update it so it conforms to the API spec you've updated? Thanks!

@stefannibrasil
Copy link
Contributor Author

oh, I see! thanks for the link, I'll check it out this week!

@jywarren
Copy link
Member

jywarren commented Nov 9, 2018

Can't seem to get codeclimate to run, so gonna close and reopen!

Also, i changed the JS query I'd mentioned above -- here! #3940

@jywarren jywarren closed this Nov 10, 2018
@jywarren jywarren reopened this Nov 10, 2018
@ghost ghost removed the in progress label Nov 10, 2018
@ghost ghost assigned jywarren Nov 10, 2018
@ghost ghost added the in progress label Nov 10, 2018
@stefannibrasil
Copy link
Contributor Author

hi, @jywarren thanks! I was going to take a look at it this holiday. Do you think this is ready, then? Thanks!

@jywarren
Copy link
Member

Ah, i think we just have to preserve the blurred attribute as mentioned above, and then it's good to go. Thanks! #3700 (comment)

@jywarren
Copy link
Member

I see the error:


ERROR["test_search_Tag_Nearby_Nodes_functionality_with_a_valid_query", #<Minitest::Reporters::Suite:0x000055902d927d00 @name="SearchApiTest">, 81.07464563299993]
 test_search_Tag_Nearby_Nodes_functionality_with_a_valid_query#SearchApiTest (81.07s)
NoMethodError:         NoMethodError: undefined method `blurred?' for #<Node:0x000055902d9d88f8>
            app/api/srch/search.rb:247:in `block (3 levels) in <class:Search>'

So, I think we need to add a blurred? method to nodes, like this one for users:

plots2/app/models/user.rb

Lines 206 to 208 in 788ff6f

def blurred?
has_power_tag("location") && get_value_of_power_tag("location") == 'blurred'
end

I'll do that now.

@jywarren
Copy link
Member

Cool, added that -- let's see: stefannibrasil@75724f3

@jywarren
Copy link
Member

Yay!!!

@jywarren jywarren merged commit d03fad1 into publiclab:master Nov 13, 2018
@ghost ghost removed the in progress label Nov 13, 2018
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
…clab#3597 (publiclab#3700)

* Update docs and add tests for the taglocations API method - fix publiclab#3597

* This is not the place for this

Removing blurred: false because it can be accessed via `user.blurred?`

* re-add blurred to the API call

*   add node.blurred?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline map API calls a bit broken?
3 participants