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

City not always located #16

Closed
spin0us opened this issue Feb 23, 2022 · 22 comments
Closed

City not always located #16

spin0us opened this issue Feb 23, 2022 · 22 comments

Comments

@spin0us
Copy link
Contributor

spin0us commented Feb 23, 2022

First of all, thank you for your work. We use your plugin on our forum and our users are very happy with it.
Lately we have noticed that some cities do not show up on the map display. For example, "Liège" in Belgium does not show up, and the same goes for "Aimargues" in France. We have tried several ways to write them, without success.
Do you have any idea about the source of this problem?
When is the geocoding of the city done? and via which api?

@julianlam
Copy link
Collaborator

julianlam commented Feb 23, 2022

The geocoding is done via the mapbox api: https://docs.mapbox.com/playground/geocoding/

It could be that Liège or Aimargues does not show up in the results because it needs more identifying information (e.g. Liège, Liège, Belgium)?

Not certain as to why, but perhaps this will help give you some indication toward a fix.

@spin0us
Copy link
Contributor Author

spin0us commented Feb 23, 2022

I just tested and it still doesn't work, even specifying Liège, Liège, Belgium or Liège, Belgium or Liège, Belgique, and so on, nothing appears on the map.
Would it be possible to add some options in the ACP such as "limit number of items to return" and "language"?

@spin0us
Copy link
Contributor Author

spin0us commented Feb 23, 2022

I just did some strange new tests. When I put "Liege" on another profile the dot appears correctly. It seems that the problem occurs only on some user accounts.
I'm a bit confused about this.

@spin0us
Copy link
Contributor Author

spin0us commented Feb 25, 2022

I'm trying to make progress on the investigation. Here is a capture of the response to the POST request when the profile was updated.

Capture d’écran 2022-02-25 à 17 38 01

The coordinates appear to be geocoded correctly, so the problem would be when the marker is displayed on the map.

@julianlam
Copy link
Collaborator

@spin0us Well, here's something odd... I tried to reproduce, but was unable to. Both Aimargues and Liège appear fine on the map 🤔

(Although I did update the plugin to be compatible with NodeBB v2.0.0, but I can't imagine that'd make much of a difference...)

@spin0us
Copy link
Contributor Author

spin0us commented Feb 28, 2022

I will try to debug this display problem by logging the users in the loop. But for that I have to install a copy of my production. I will make a return here...

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

@julianlam Apparently ajaxify.data.users does not contain all users. This explains that whatever the city, some users do not appear on the map. This is quite strange. I only have 56 users that show up via this object (150+ registered users).

@julianlam
Copy link
Collaborator

Well, that sounds promising, thanks for doing the debugging legwork!

@julianlam
Copy link
Collaborator

julianlam commented Mar 1, 2022

@spin0us latest master I refactored renderMap for readability. Can you install 558a1ff and manually add some debugging? You can install just that commit by doing npm install https://github.com/NodeBB-Community/nodebb-plugin-osm-map#558a1ff I think.

https://github.com/NodeBB-Community/nodebb-plugin-osm-map/blob/558a1ff/library.js#L20-L28

Can you add console.log(users.length) before and after the users = users.filter()... line? I'd like to know whether users are missing from the osmMap.users set, or whether they're filtered out because they're missing the lat/lon pair.

@julianlam
Copy link
Collaborator

One other thing to try, is to have the affected users clear the "Location" field, save, and then set one again. Perhaps that could resolve.

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

@spin0us latest master I refactored renderMap for readability. Can you install 558a1ff and manually add some debugging? You can install just that commit by doing npm install https://github.com/NodeBB-Community/nodebb-plugin-osm-map#558a1ff I think.

https://github.com/NodeBB-Community/nodebb-plugin-osm-map/blob/558a1ff/library.js#L20-L28

Can you add console.log(users.length) before and after the users = users.filter()... line? I'd like to know whether users are missing from the osmMap.users set, or whether they're filtered out because they're missing the lat/lon pair.

before 57
after 57

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

One other thing to try, is to have the affected users clear the "Location" field, save, and then set one again. Perhaps that could resolve.

No change with this action.

@julianlam
Copy link
Collaborator

  • Scroll down to osmMap.addCoordinates
  • Add console.log(lon, lat, !!lat && !!lon); inside setLonLat
  • Add console.log(profile.data); right before the if conditional

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

  • Scroll down to osmMap.addCoordinates
  • Add console.log(lon, lat, !!lat && !!lon); inside setLonLat
  • Add console.log(profile.data); right before the if conditional

Here is the result for the missing user on map when i update his city :

{
  fullname: '',
  website: '',
  location: 'Aimargues',
  birthday: '',
  aboutme: '',
  signature: '',
  uid: '3',
  groupTitle: '[null]'
}
4.20861 43.68472 true

@julianlam
Copy link
Collaborator

Interesting... undo all that, let's go back up to renderMap

  • Add console.log(uids.length, uids.includes('3') after const uids = line

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

Interesting... undo all that, let's go back up to renderMap

  • Add console.log(uids.length, uids.includes('3') after const uids = line

That's just the test I was doing. I used a console.log('uids', uids); and here is the result :

uids [
  '2',   '5',   '6',   '8',   '9',   '11',  '12',
  '13',  '16',  '18',  '20',  '22',  '25',  '26',
  '29',  '30',  '31',  '32',  '33',  '34',  '35',
  '36',  '37',  '39',  '42',  '43',  '45',  '46',
  '47',  '48',  '49',  '51',  '56',  '57',  '58',
  '61',  '70',  '72',  '76',  '77',  '78',  '88',
  '89',  '90',  '98',  '101', '102', '103', '104',
  '107', '108', '119', '134', '14',  '145', '146',
  '4'
]

As you can see, no 3 in the list.

@julianlam
Copy link
Collaborator

Very weird. You may need to debug why whenever the uid is 3, db.setAdd is not called inside osmMap.addCoordinates.

Or if it is, then why it isn't actually saving to the db 🤔

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

Very weird. You may need to debug why whenever the uid is 3, db.setAdd is not called inside osmMap.addCoordinates.

Or if it is, then why it isn't actually saving to the db thinking

I think i got it :

{
  uid: 4,
  data: {
    fullname: '',
    website: '',
    location: 'Aimargues',
    birthday: '',
    aboutme: '',
    signature: '',
    uid: '3',
    groupTitle: '[null]'
  },
  fields: [
    'username', 'email',
    'fullname', 'website',
    'location', 'groupTitle',
    'birthday', 'signature',
    'aboutme'
  ],
  caller: { uid: 4 }
}

await (!!lat && !!lon ? db.setAdd : db.setRemove)('osmMap.users', profile.uid); is done on profile.uid instead of profile.data.uid

Is it correct ?

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

I've made the change from await (!!lat && !!lon ? db.setAdd : db.setRemove)('osmMap.users', profile.uid); to await (!!lat && !!lon ? db.setAdd : db.setRemove)('osmMap.users', profile.data.uid); and it's working now.

So the bug was when an admin update a member profile it does not insert the member uid but the admin uid.

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

I tried to create a pull request for this.

#20

@spin0us spin0us closed this as completed Mar 1, 2022
@julianlam
Copy link
Collaborator

Thanks! That said, if the user updated their own profile, it should work, no?

@spin0us
Copy link
Contributor Author

spin0us commented Mar 1, 2022

Yes, indeed

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

No branches or pull requests

2 participants