search

hippware / rn-chat

MIT License
5 stars 0 forks source link

Prevent embedded location data points for bot operations if invisible/hidden. #4909

Closed bengtan closed 4 years ago

bengtan commented 4 years ago

This is a follow-up to #4881 / #4908 to also prevent posting of embedded location data points if invisible. This one applies to bot related operations.

However, it ended up being more complicated than I thought, and some of the changes might be controversial.

Hence, please review but don't merge yet. I wanna think over my own changes too.

aksonov commented 4 years ago

Hm.. This PR disables sending location (bot invite) for all modes even for 'normal', not 'invisible', right?

bengtan commented 4 years ago

Re-rolled this PR post #4906.

This PR also tidies up bot.userLocation which is no longer needed since location can be retrieved from wocky.profile. Actually, this is the majority of the changes.

Hm.. This PR disables sending location (bot invite) for all modes even for 'normal', not 'invisible', right?

Disagree.

If the mode is 'normal', location is still embedded, but it's retrieved from wocky.profile.location instead of from the bot's userLocation.

Or have I misunderstood your question?

aksonov commented 4 years ago

As I remember userLocation was used for cases when user is inside bot already but no location update will be sent if he is not moving.

bengtan commented 4 years ago

As I remember userLocation was used for cases when user is inside bot already but no location update will be sent if he is not moving.

Yes, but that was from a long time ago when ownProfile didn't have a location (or something like that). The location was retrieved from LocationStore and cached into the bot (as userLocation).

Remember when ownProfile did have it's own location but it also duplicated LocationStore.location and then we did some refactoring to make one a computed derivative of the other?

After the refactoring, I don't think we need to cache location into bot.userLocation anymore. So AFAICT, userLocation is no longer necessary.

90% of this PR is removing userLocation.

Alternatively, if we choose not to remove userLocation, I can redo this PR and leave userLocation untouched. It'll just not send embedded location when the user is invisible/hidden. It would be a much smaller PR.

aksonov commented 4 years ago

I'm fine to merge it, we always could revert if QA creates the ticket