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

Allow choosing to not use a mailserver #11035

Closed
hesterbruikman opened this issue Aug 5, 2020 · 12 comments · Fixed by #11296
Closed

Allow choosing to not use a mailserver #11035

hesterbruikman opened this issue Aug 5, 2020 · 12 comments · Fixed by #11296
Milestone

Comments

@hesterbruikman
Copy link
Contributor

hesterbruikman commented Aug 5, 2020

Problem

From a privacy perspective, it's important, for the user to be able to opt out of using a mailserver to collect history (i.e. messages sent while you were offline).

Implementation

Designs to be added (@errorists can you please add the final design in the comments?)

Acceptance Criteria

User is able to disable use of a mailserver

Notes

Giving the user control makes a stronger case in terms of privacy policy that the user controls their personal data (IP address)

Future Steps

@errorists
Copy link
Contributor

Designs can be found here, I renamed all instances of mailserver to history node there following Hester's comments. The lowest hanging fruit type scope way to do this is to add the switch to Enable / Disable like on the designs.

@shivekkhurana
Copy link
Contributor

shivekkhurana commented Sep 12, 2020

@hesterbruikman
I'm working on this bounty and have created the UI (code pushed to my forked branch). It:

  • Adds new element for disabling mail server
  • Converts the Auto selection checkbox to switch
  • Filters the servers that are visible depending on auto selection state

I now need to handle the disable history, which I'm guessing is an API call. Is there any documentation where I can check the API spec? Or any sample code that's already in place?

Thanks

@errorists
The new UI looks polished, but there are two issues I faced:

  • Server selection UI uses radio buttons on the right, but in the current UI radio buttons are on the left . Are you hoping for a global change where radio buttons show up on the right? I found an implementation of radio buttons on right side. Will make changes to the UI.
  • The Automatic Selection Switch was designed with bold title. However the component available doesn't allow for that (I might be wrong here). I have taken the liberty to adjust the designs, however this is not final and I'm open to implement changes if you suggest:

Screenshot 2020-09-12 at 2 20 48 PM

Screenshot 2020-09-12 at 2 20 59 PM

@TeoChiriac
Copy link

Hi Shivek,

Thanks for being so prompt with the task.

Can any of these two links help? If you can't find the information you need in the technical documentation, please let me know so I can flag to the team.

https://status.im/developer_tools/status_web_api.html

https://status.im/developer_tools/status_extras/index.html

@shivekkhurana
Copy link
Contributor

I checked the links you sent, but I don't think it will help.
I'm not even sure if I need to make an API call or not.

@jakubgs
Copy link
Member

jakubgs commented Sep 13, 2020

This file contains a list of JSON RPC calls available from within JS side of status-react repo currently:
https://github.com/status-im/status-react/blob/32cb09c9c68497ce152216067d2e00309baef686/src/status_im/ethereum/json_rpc.cljs#L12-L216
Here is the source of those API calls within the status-go repo:
https://github.com/status-im/status-go/blob/develop/services/ext/api.go
These calls are an extension of the default JSON RPC API provided by go-ethereum:
https://eth.wiki/json-rpc/API
My bet would be on the wakuext_updateMailservers call, but not sure:
https://github.com/status-im/status-react/blob/32cb09c9c68497ce152216067d2e00309baef686/src/status_im/ethereum/json_rpc.cljs#L152
https://github.com/status-im/status-go/blob/8f83c5f462de854fa9b616273ccfdd4b0bda4748/services/ext/api.go#L415-L425

It might be that there is no call that will fit your needs and this change will require adding it to status-go.
@cammellos or @Samyoul would be the people to talk about this if that's the case.

@shivekkhurana
Copy link
Contributor

Thanks for the input @jakubgs.

What is the difference between wakuext_ and shhext_ calls?

The function responsible for updating the MailServer expects a list:
https://github.com/status-im/status-go/blob/8f83c5f462de854fa9b616273ccfdd4b0bda4748/services/ext/api.go#L415-L425

I'll try to send an empty list to check if it works. But a confirmation from the backend team will be helpful. I have no clue how the system works and am just guessing that selecting 0 mailservers will stop the sync process.

Also, having seen the API, does anyone thinks that renaming to "Mailserver" to "History Nodes" will create confusion or is everyone okay with incremental renaming?

@cammellos
Copy link
Contributor

Hi @shivekkhurana
thanks for looking at this issue.

What is the difference between wakuext_ and shhext_ calls?
The difference between wakuext_ and shhext_ is based on whether you have waku or whisper enabled.

For the purpose of this task you probably don't need to call updateMailservers, it is only used once you are connected to one and signal to status-go that we want to receive message confirmations (I can elaborate on it, but you don't need to be worried about this in this task) from those mailservers.

Essentially you need to make sure that connect-to-mailserver ( https://github.com/status-im/status-react/blob/32cb09c9c68497ce152216067d2e00309baef686/src/status_im/mailserver/core.cljs#L221 ) is never called (or better, is called but noops), if the user has not enabled mailservers.
Once you do that, it might show some "connecting..." dialog as the app expects to be connected to a mailserver, and that's something that should not be shown if the user has not enabled mailservers.

Also, having seen the API, does anyone thinks that renaming to "Mailserver" to "History Nodes" will create confusion or is everyone okay with incremental renaming?
This is a hotly debated topic :) I don't have a strong opinion myself, I defer to UX on the call cc @hesterbruikman @errorists

This also will require some changes on status-go, as the setting needs to be persisted, I can help you out with that (of course you are free to work on it if you wish), for now you can pick a variable name and just update the re-frame db (similarly to what we do with syncing-on-mobile-network? for example).

Hope this is helpful, and ask any question, we're happy to help (you can ping us on discord as well if you want to have a quick chat).

@errorists
Copy link
Contributor

errorists commented Sep 14, 2020

@shivekkhurana

The Automatic Selection Switch was designed with bold title

it's a Font weight: 500 and that's the default if you use a List Item component with a subtitle present, like the one on the design. Looks fine on the screenshots

@shivekkhurana
Copy link
Contributor

Thanks guys. I have fixed the UI to match @errorists designs and made the use-history-nodes? switch work:

  • Radio buttons instead of icons on left
  • Switch instead of checkbox
  • Add button with round border instead of just the add symbol
  • Use history switch state is saved only locally
  • Use history is not functional, only the UI works
Screenshot 2020-09-14 at 3 25 26 PM Screenshot 2020-09-14 at 3 25 20 PM
Screenshot 2020-09-14 at 3 25 34 PM

🔗 My branch

@errorists
Copy link
Contributor

@shivekkhurana looks good! nice work on the UI

@cammellos
Copy link
Contributor

@shivekkhurana I have updated status-go,
you can run the latest version by:

make shell
scripts/update-status-go.sh 'feature/use-mailservers'

And then restart the stack (it will re-compile status-go, so it might take some time)

The last thing left to do is to actually prevent https://github.com/status-im/status-react/blob/32cb09c9c68497ce152216067d2e00309baef686/src/status_im/mailserver/core.cljs#L221
this function to be called when the settings are on.
Amazing work on the UI!
Let me know if there's any issue/question.

@cammellos
Copy link
Contributor

Ah, I have also named the variable use-mailservers? instead of use-history-nodes?for consistency, but we can keepHistory nodes` on the translation if that's preferred by ux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment