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

[feature] Add geojson endpoints #360 #374

Merged
merged 11 commits into from
Feb 18, 2021

Conversation

purhan
Copy link
Contributor

@purhan purhan commented Jan 27, 2021

Closes #360

@purhan

This comment has been minimized.

@purhan purhan force-pushed the issues/360-add-geojson-api-endpoint branch from a6f6b95 to 5a22e0d Compare January 27, 2021 14:39
@purhan purhan self-assigned this Jan 27, 2021
@purhan purhan force-pushed the issues/360-add-geojson-api-endpoint branch from 5a22e0d to a41a5fd Compare January 27, 2021 14:58
@purhan purhan force-pushed the issues/360-add-geojson-api-endpoint branch from 1e75dca to e841656 Compare February 2, 2021 11:08
@purhan purhan force-pushed the issues/360-add-geojson-api-endpoint branch 2 times, most recently from 31ea6a7 to 74553b0 Compare February 2, 2021 15:35
@purhan purhan marked this pull request as ready for review February 2, 2021 16:37
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great progress @purhan!

I tried opening the geojson URL without being logged in and got this exception:

'AnonymousUser' object has no attribute 'organizations_managed'

Is this a problem of these views or is it an underlying issue of the filtering classes in openwisp-users?

Ensure the views are accessible only to authenticated users.

The rest looks good but I will spend some more time testing it, see also my comment below.

openwisp_controller/geo/utils.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 3, 2021

Coverage Status

Coverage increased (+0.02%) to 99.105% when pulling 771b14e on Purhan:issues/360-add-geojson-api-endpoint into 45af289 on openwisp:master.

@purhan
Copy link
Contributor Author

purhan commented Feb 3, 2021

Great progress @purhan!

I tried opening the geojson URL without being logged in and got this exception:

'AnonymousUser' object has no attribute 'organizations_managed'

Is this a problem of these views or is it an underlying issue of the filtering classes in openwisp-users?

Ensure the views are accessible only to authenticated users.

The rest looks good but I will spend some more time testing it, see also my comment below.

@nemesisdesign I've fixed it by adding a permission class to allow only the authenticated users to access it. I think we should add this functionality in the filter classes themselves. What do you think?

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great progress @purhan!
I tried opening the geojson URL without being logged in and got this exception:

'AnonymousUser' object has no attribute 'organizations_managed'

Is this a problem of these views or is it an underlying issue of the filtering classes in openwisp-users?
Ensure the views are accessible only to authenticated users.
The rest looks good but I will spend some more time testing it, see also my comment below.

@nemesisdesign I've fixed it by adding a permission class to allow only the authenticated users to access it. I think we should add this functionality in the filter classes themselves. What do you think?

Yes I think you're right @purhan.

One more request, I forgot to mention it before, can you add a brief mention of this endpoint in the README? https://github.com/openwisp/openwisp-controller#geo-app

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

For the rest looks good!

@purhan
Copy link
Contributor Author

purhan commented Feb 9, 2021

One more request, I forgot to mention it before, can you add a brief mention of this endpoint in the README? https://github.com/openwisp/openwisp-controller#geo-app

Done

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

almost there!

requirements.txt Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great work @purhan 👍!

@nemesifier nemesifier merged commit 7632598 into openwisp:master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

[feature] Add GeoJSON API endpoint with device location data
3 participants