Page MenuHomeWildfire Games

Extract the terms code from the prelobby to reuse it for other term accepters (userreport)
ClosedPublic

Authored by bb on Aug 3 2018, 12:52 AM.

Details

Summary

To avoid duplication move the terms code from the prelobby, to its own page and gui/common so we can use it in other places too (user report).

Use Accept/Decline buttons instead of the checkbox

This allows adding a language dropdown for the window without duplication.

Test Plan

See the non-added duplication gone
Agree on the new design

Notice mod.io is sortof a different story since

  1. we are not the gdpr controller
  2. we need the links ingame

Solve the terms.js overflow
Yell about prototype hides if wanted

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bb created this revision.Aug 3 2018, 12:52 AM
Owners added a subscriber: Restricted Owners Package.Aug 3 2018, 12:52 AM
Vulcan added a subscriber: Vulcan.Aug 3 2018, 12:56 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/701/display/redirect

bb updated this revision to Diff 6842.Aug 5 2018, 5:53 PM

Don't use a common/ global outside common/

Vulcan added a comment.Aug 5 2018, 6:41 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/common/terms/terms.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/common/terms/terms.js
|  17|  17| 			"hashPrefixObject": "username",
|  18|  18| 			"accept": false
|  19|  19| 		}
|  20|    |-	})
|    |  20|+	});
|  21|  21| };
|  22|  22| 
|  23|  23| function updateTermsFeedback()
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/common/terms/terms.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/common/terms/terms.js
|  18|  18| 			"accept": false
|  19|  19| 		}
|  20|  20| 	})
|  21|    |-};
|    |  21|+}
|  22|  22| 
|  23|  23| function updateTermsFeedback()
|  24|  24| {

binaries/data/mods/public/gui/prelobby/common/terms/terms.js
|  20| »   })
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/prelobby/common/terms/terms.js
|  21| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/register/register.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/register/register.js
|   4|   4| 
|   5|   5| 	Engine.GetGUIObjectByName("continue").caption = translate("Register");
|   6|   6| 
|   7|    |-	initLobbyTerms()
|    |   7|+	initLobbyTerms();
|   8|   8| 
|   9|   9| 	initRememberPassword();
|  10|  10| 

binaries/data/mods/public/gui/prelobby/register/register.js
|   7| »   initLobbyTerms()
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/login/login.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/login/login.js
|   8|   8| 	Engine.GetGUIObjectByName("username").caption = Engine.ConfigDB_GetValue("user", "lobby.login");
|   9|   9| 	Engine.GetGUIObjectByName("password").caption = Engine.ConfigDB_GetValue("user", "lobby.password").substr(0, 10);
|  10|  10| 
|  11|    |-	initLobbyTerms()
|    |  11|+	initLobbyTerms();
|  12|  12| 
|  13|  13| 	loadTermsAcceptance();
|  14|  14| 	initRememberPassword();

binaries/data/mods/public/gui/prelobby/login/login.js
|  11| »   initLobbyTerms()
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

Link to build: https://jenkins.wildfiregames.com/job/differential/708/display/redirect

elexis added a subscriber: elexis.Aug 6 2018, 9:26 AM

git:
It's hard to see whether the refactored code can be reused this way with mod.io and the UserReporter.
That's why I did and do propose to use a git branch, implement it for all three (we can implement in coop-mode). Then if further refactoring is needed, the commits can be refactored too.

If the refactoring is done well, it will be easy to use it for the fourth, fifth, sixth... online service integration (rated games could be split to a separate service eventually, more integration to online services hosted by us or others such as mod.io).

acceptance saving
There is a behavior change to save the terms acceptance on tick and untick rather than after authentication.
For the multiplayer lobby we know that it is not that uncommon to have multiple players on one computer.
That's why the saving was done only after the authentification.
This made sure that every account read the terms before having connected.

In the worst form it means player1 installs the new version, accepts the changed privacy policy, (logs in and out, or not), closes the program, player2 opens the page and is not informed of the new terms and conditions.
There was already a concnern by Josh on that commit not being aware about this being a feature, not a bug. So in fact the defect is me not having written a comment explaining this.

We notice that the same problem can be projected onto the other terms and conition pages.
I guess it's a bit of a paradox if player1 wants to use the hardware reporter but player2 doesn't, since the same personal data would be generated.

For the mod.io terms, we have some with few clauses, even if we aren't a GDPR controller or processor and they might change at some time for any relevant reason.
Then all users of mod.io should be informed, not only the one touching the UI, in theory.

User-account problem:
This exposes the underlying problem of multiple accounts per computer problem by much more.
It would be much saner if there was one user.cfg for each user.
The different users may want to use different graphics, sound settings, hotkeys, lobby buddies and so forth.

At the time of the login an account would have to be chosen if there are multiple.
This doesn't really make it easier to smurf on the lobby.
Insert ticket here I guess.

For the purposes of this patch it should be ok to keep the per-account saving on the lobby.
For mod.io the dialog can always be shown as long as it's so short. Also the links to mod.ios terms should ideally always remain reachable prior to connection. (Same for the userreporter.)

Also semicolons mentioned by the bot.

Filenames:
Since there are now 3 terms.js files, these should be renamed, so that it becomes clear by their filename what's in them.
Also it's difficult to open the right file if one has to find the one folder that is different in the path.

Function name hardcoding:
IIRC In the previous prelobby refactoring commits there were hardcoded function names in the terms page that the parent function must implement. It would probably be better if the parent function would pass the function name if that is so.

binaries/data/mods/public/gui/common/terms.js
3 ↗(On Diff #6842)

In this function terms is `g_Terms.
In the second function terms is the property name of g_Terms.
↯ (contradicts reader assumption that the same name refers to the same thing)

May want to solve with a rename to pageName, termsName or termsPageName for example.

binaries/data/mods/public/gui/prelobby/common/terms/terms.js
17 ↗(On Diff #6842)

hashPrefixObject is a weird argument. It only supports getting the caption of a chat input, but the salt could be anything else by UI pages, for example config strings.
How passing the same global termsHashSalt function as a hashSalt argument?

18 ↗(On Diff #6842)

accepted (the former read was to be read as past tense too)

21 ↗(On Diff #6842)

semicolon on the wrong line

binaries/data/mods/public/gui/prelobby/register/register.js
31 ↗(On Diff #6842)

This made sure that every account read the terms before having connected.

binaries/data/mods/public/gui/terms/terms.js
11 ↗(On Diff #6842)

accepted

binaries/data/mods/public/gui/terms/terms.xml
27 ↗(On Diff #6842)

(that's where the language selection dropdown can be added to have legal english and illegal translations)

elexis added a comment.Aug 7 2018, 2:14 PM

Actually because the terms-acceptance is reloaded if the username is edited, there is no regression here. (Underlying problem stills lies down under there, per-user and per-mod configs are probably cleaner).

The review remarks, language selection dropdowns and use of the 3 terms uses were all implemented by us in https://github.com/bb-bb/0ad/tree/terms

It seems we only need the documents themselves.

elexis accepted this revision.Sep 14 2018, 5:14 PM
elexis added inline comments.
binaries/data/mods/public/gui/common/terms.js
1 ↗(On Diff #6842)

moved from public to mod, as it must be reusable without public launched for modio

binaries/data/mods/public/gui/prelobby/register/register.js
17 ↗(On Diff #6842)

(this hunk was a bugfix and committed already)

binaries/data/mods/public/gui/terms/terms.xml
25 ↗(On Diff #6842)

Strings ok, but they will be committed with all the other strings in one commit - using the existing Cancel and Connect now, which is a bit silly but will be changed in a commit afterwards with the other strings.

binaries/data/mods/public/l10n/messages.json
288 ↗(On Diff #6842)

None of the messages.json files need any change as the termsdialog must be present in the "mod" mod, and mod/messages.json extracts everything already

This revision is now accepted and ready to land.Sep 14 2018, 5:14 PM
This revision was automatically updated to reflect the committed changes.