Skip to content

Conversation

@esirK
Copy link
Contributor

@esirK esirK commented Feb 4, 2021

Description

Adds management command to reverse geocode nodes locations and update the city names for nodes with no city associated with them

Fixes # (issue)

Type of change

  • New feature (non-breaking change which adds functionality)

Screenshots

Screenshot 2021-02-04 at 13 48 07

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@esirK esirK added the enhancement New feature or request label Feb 4, 2021
@esirK esirK self-assigned this Feb 4, 2021
Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

👍 @esirK

  1. Why do we need the geo reversing in the api itself? Each function responsible of pushing data should provide correct location moving foward, right?
  2. Shouldn't cities be part of location with countries in meta?

@esirK esirK force-pushed the ft-sensors-cities branch from 7e216eb to 461d240 Compare February 4, 2021 10:48
@esirK
Copy link
Contributor Author

esirK commented Feb 4, 2021

  1. Why do we need the geo reversing in the api itself? Each function responsible of pushing data should provide correct location moving foward, right?

Yes moving forward we expect that. But we aren't guaranteed that will always be the case so I'm just leaving the management command here just in case anyone needs it in the future.

Shouldn't cities be part of location with countries in meta?

I've attached the screen shot to show how it will look like. I asked @KobbyMmo before adding sensors_cities as their own object

@esirK esirK requested a review from kilemensi February 4, 2021 10:56
@kilemensi
Copy link
Member

Shouldn't cities be part of location with countries in meta?

I've attached the screen shot to show how it will look like. I asked @KobbyMmo before adding sensors_cities as their own object

Nah, you can't have sensors_locations to mean countries only and have separate sensors_cities... either have sensors_countries and sensors_cities as standalone objects or one sensors_locations return countries and their count as well as cities and their count.

It should even be cities in their respective countries but it's too late for today I think.

@KobbyMmo
Copy link

KobbyMmo commented Feb 4, 2021

Yea @esirK , what you have will work for me

This will also be fine.

location:{
      cities:{
        count: 12, data: []
      },
      countries:{
        count: 12, data: []
      }
}

@esirK
Copy link
Contributor Author

esirK commented Feb 4, 2021

@KobbyMmo I'll stick to what we have for now. The only thing you will change is using sensors_countries instead of sensors_locations

@esirK esirK merged commit 2a6a201 into master Feb 4, 2021
@esirK esirK deleted the ft-sensors-cities branch February 4, 2021 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants