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

Gdpr compliance #3748

Merged
merged 4 commits into from
Feb 9, 2019
Merged

Gdpr compliance #3748

merged 4 commits into from
Feb 9, 2019

Conversation

hiker
Copy link
Contributor

@hiker hiker commented Feb 8, 2019

First fix for #3378: sending hardware stats is now opt-in (no default to send hardware stats anymore). Fixes also incorrect help message and incorrect status display if hw-stats are reported or not, and made the chat option (in)visible same as hw-stats.

Note that the order of entries in the 'internet' option is strange (the 'always show login' option is not strictly an internet-only option, since it also affects local-only accounts), but I am not sure why this order was chosen, so I left this unchanged.

Combining internet-access and hw-stats with just one option is bad now that stk is gdpr compliant, since it must now default to internet-off :( But I can't see an easy and convenient location - best option imho: remove the HW statistic, since we are actually not using the stats anymore.

hiker added 4 commits February 8, 2019 15:10
This means atm that even connection to internet is disabled as default :(
it was actually showing the state of the 'chat', not 'hw-stats'.
enabled in the options (GDPR compliance - opt-in required from
user to send data). Made chat-message visible/invisible same
as hw-statistics.
"Internet\" and \"Send anonymous HW statistics\")."),
MessageDialog::MESSAGE_DIALOG_YESNO,
new ConfirmServer(), true, true, 0.7f, 0.7f);

// Changes the default focus to be 'cancel', which is not
Copy link
Collaborator

Choose a reason for hiding this comment

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

You meant that the current "ok" default focus is not compliant ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so - gdpr requires a deliberate choice by the user to opt-in. With 'ok' being the default, and assuming that just pressing enter is not a deliberate choice (given that opt-out requires a click and pressing enter). IANAL, and if anyone can point out a counter example I am happy to leave the default, I don't like this.
We could add e.g. internet-permission as part of the installation process, but that's a lot of work and only covers a part of our user base :(
TBH, if we agree that we don't need the HW stats (has anyone looked at them in the last two years??), taking this out would be a really good choice imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

@leyyin had disabled stats viewing because it was OOMing the old VPS, but I think since then IIRC the VPS got more available RAM. @leyyin Can you re-enable stats viewing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hiker The comment is wrong, it says the new default is not compliant

Copy link
Member

@vampy vampy Feb 9, 2019

Choose a reason for hiding this comment

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

Unfortunately I can't because the generation script is badly optimized it needs more than 1 GB of ram.

Copy link
Contributor

Choose a reason for hiding this comment

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

@leyyin I thought the VPS got 2GB RAM a little while back as the provider upgraded the plans? Also is the script in a github repo so someone can work on it? Stats are useless if we can't use them, this needs to be fixed.

Or maybe just buy a VPS with more RAM, AFAIK we're not short cash for servers.

Copy link
Member

@vampy vampy Feb 10, 2019

Choose a reason for hiding this comment

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

It got an upgrade to only 1 GB of ram.

As for the scripts, see:

Buying more servers won't be a solution, fixing the code is.
Fix software not buy more hardware ;)

@qwertychouskie
Copy link
Contributor

Also see #2549, this would fix the internet disabled by default issue by allowing independent control.

@deveee deveee merged commit 94d6a6f into master Feb 9, 2019
@deveee
Copy link
Member

deveee commented Feb 9, 2019

Merged. And I think that we don't need HW stats too.

@Alayan-stk-2
Copy link
Collaborator

We don't make use of hw stats and can't access them because of server issues, but it could definitely come handy. For example, the share of old processors and old igpu used to run STK can help to guide hw requirements.

Also, some opt-in for general gameplay data collection would be useful so axing hw stats wouldn't make a lot of sense in that context imho.

@hiker
Copy link
Contributor Author

hiker commented Feb 11, 2019

@Alayan-stk-2 good catch with the comment, thanks. I've just pushed the fix to master.

The other discussion: I would recommend to put this in a separate issue to track the decision about HW stats if required :)

@deveee deveee deleted the gdpr-compliance branch February 15, 2019 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants