Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

[WIP-GSoC-19] Store Livechat sessions on backend side #242

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

knrt10
Copy link
Contributor

@knrt10 knrt10 commented Jun 8, 2019

@renatobecker please review

package.json Outdated Show resolved Hide resolved
src/components/App/index.js Show resolved Hide resolved
@reetp
Copy link

reetp commented Jun 14, 2019

Yes - this must not be automatically implemented, must have an off switch set to disabled by default, and be separately switchable from other information

eg you may wish to collect an email address but NOT geo location data

There must be some way to notify users of the data collection if it is enabled.

@knrt10 knrt10 force-pushed the iplocation branch 2 times, most recently from 41cf4dd to 1608fba Compare June 16, 2019 14:38
@renatobecker-zz renatobecker-zz changed the title setting location Data for user [WIP-GSoC-19] setting location Data for user Jun 17, 2019
src/components/App/index.js Outdated Show resolved Hide resolved
src/components/App/index.js Outdated Show resolved Hide resolved
src/components/App/index.js Outdated Show resolved Hide resolved
src/components/App/index.js Outdated Show resolved Hide resolved
src/widget.js Outdated Show resolved Hide resolved
* @returns {Object}
*/
const locationPrimary = async(latitude, longitude) => {
const { address } = await fetch(`https://nominatim.openstreetmap.org/reverse?format=json&lat=${ latitude }&lon=${ longitude }`, {

Choose a reason for hiding this comment

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

So this is going out to this api to grab location? This has to definitely be optional if done at all. Some serious privacy concerns around sending data like this out.

* @returns {Object}
*/
const locationBackup = async() => {
const location = await fetch('https://api.ipdata.co?api-key=test', {

Choose a reason for hiding this comment

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

Same here

@reetp
Copy link

reetp commented Aug 2, 2019

Apart from truck loads of GDPR concerns here this, wording has a typo.

"please_enable_your_location_to_start_chat_this_is__c68e0f77": "Please enable your location to start chat. This is for your convinience only. Thanks",

"Please enable your location to start chat. This is for your convenience only. Thanks"

It also sounds like you are telling users that unless they enable location they cannot chat with you?

I don't think so.

  • You have to ask for permission before you take any data. You can't do it in the background and then say "oh please let me use this"

  • If you want to insist on obtaining location data before chatting then it needs an off switch for those that do not want to impose this

  • You have to be able to remove their data on request if you are storing it

@renatobecker-zz renatobecker-zz changed the title [WIP-GSoC-19] setting location Data for user [WIP-GSoC-19] Store Livechat sessions on backend side Aug 25, 2019
src/api.js Outdated
|| queryString.parse(window.location.search).serverUrl
|| (process.env.NODE_ENV === 'development' ? 'http://localhost:3000' : null);
// eslint-disable-next-line import/no-absolute-path
import LivechatClient from '/Users/knrt10/dev/openSource/RocketChat/Rocket.Chat.js.SDK/dist/lib/clients/Livechat';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forget to remove this code before pushing your changes.

@@ -0,0 +1,290 @@
/* eslint-disable no-lonely-if */
/* eslint-disable no-alert */
import { getToken } from './main';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a clone from the userPresence lib, you don't need a new lib to do the same things, it does not make sense.
You can do what you want by calling the new methods related to session stuff within the userPresence lib.
Please, remove this file and move the new methods to a helper lib.

@@ -106,13 +106,15 @@ export class ChatContainer extends Component {
this.stopTypingDebounced.stop();
await Promise.all([
this.stopTyping({ rid, username: user.username }),
Livechat.updateSessionStatus('online', token),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why call this method here?
We have a specific lib to deal with user status.

@@ -30,6 +30,7 @@ export class RegisterContainer extends Component {
await dispatch({ loading: true, department });
try {
await Livechat.grantVisitor({ visitor: { ...fields, token } });
await Livechat.updateVisitorSessionOnRegister({ visitor: { ...fields, token } });
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just one method to update the livechat session?
You just need one method, you can pass specific data as parameters.

@@ -149,6 +151,7 @@ export class App extends Component {
async finalize() {
CustomFields.reset();
userPresence.reset();
userSessionPresence.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this lib.

@renatobecker-zz
Copy link
Contributor

renatobecker-zz commented Aug 25, 2019

@knrt10
There is a lot of work to do here.

  • We just need one new method on the SDK, I suggest to call it as Livechat.updateVisitorSession.
  • Regardless of whether you submit the user's location or not, you can pass all information through the SDK method parameter, you don't need to create any other method;
  • You need to create a new setting on the backend side which will be used to allow/deny ask the user to collect its location on the widget side;

Please, keep in mind you need to reduce the complexity in this PR, one suggestion is to use just one method to handle the session data.
Feel free to ping me in case any of my comments are not clear for you.

@engelgabriel
Copy link
Member

@tassoevan tassoevan changed the base branch from dev to develop October 11, 2019 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants