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

feat: added a geoip api endpoint #10648

Merged
merged 5 commits into from
Aug 7, 2024

Conversation

4nt0ineB
Copy link
Member

@4nt0ineB 4nt0ineB commented Aug 6, 2024

What

  • Expose a GeoIP API endpoint
  • some refactor in the API.pm

Screenshot

image

Related issue(s) and discussion

@github-actions github-actions bot added API Issues related to the Open Food Facts API. More specific labels exist & should be used (API WRITE…) 📍 geolite labels Aug 6, 2024
@4nt0ineB
Copy link
Member Author

4nt0ineB commented Aug 6, 2024

  • The endpoint is totally public, is that ok ?
  • I put the endpoint handler in the GeoIP.pm. Would it be better to have a dedicated module ? There are already APIProduct(Read/Revert/Services), etc.

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 8.69565% with 21 lines in your changes missing coverage. Please review.

Project coverage is 49.33%. Comparing base (dc04d18) to head (0ad5999).
Report is 544 commits behind head on main.

Files Patch % Lines
lib/ProductOpener/GeoIP.pm 0.00% 13 Missing ⚠️
lib/ProductOpener/API.pm 20.00% 4 Missing ⚠️
lib/ProductOpener/Routing.pm 20.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10648      +/-   ##
==========================================
- Coverage   49.54%   49.33%   -0.21%     
==========================================
  Files          67       77      +10     
  Lines       20650    22009    +1359     
  Branches     4980     5254     +274     
==========================================
+ Hits        10231    10859     +628     
- Misses       9131     9825     +694     
- Partials     1288     1325      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teolemon teolemon requested a review from g123k August 6, 2024 12:19
@4nt0ineB 4nt0ineB marked this pull request as ready for review August 6, 2024 14:35
@4nt0ineB 4nt0ineB requested a review from a team as a code owner August 6, 2024 14:35
Copy link

sonarqubecloud bot commented Aug 7, 2024

@stephanegigandet
Copy link
Contributor

@4nt0ineB what is the need for the GeoIP endpoint? Who is going to use it? Is it only for internal use (if so, which project?).

@teolemon
Copy link
Member

teolemon commented Aug 7, 2024

There's a link to the issue: #10594

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -384,6 +385,43 @@ Reference to the request object.

=cut

# Dipatch table for API actions
my $dispatch_table = {
Copy link
Member

Choose a reason for hiding this comment

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

Great refactor 👍

@stephanegigandet stephanegigandet merged commit a0442af into openfoodfacts:main Aug 7, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Issues related to the Open Food Facts API. More specific labels exist & should be used (API WRITE…) 📍 geolite 🧪 tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose the GeoIP API for the mobile app
5 participants