Paths

Table of Contentst

Diffusion 0 A.D. Public Repository rP21847

Rewrite the prelobby pages and add the Terms of Service, Terms of Use and the…
AuditedrP21847
Actions

Description

Rewrite the prelobby pages and add the Terms of Service, Terms of Use and the agreement checkbox to the login page.
Ensure lobby players cannot join without acceptance of the terms in case they change, fixes #5218, as agreed in last staff meeting.

Separate lobby entrance, lobby login and lobby register GUI page.
Since rP14098 they were squashed into a single GUI page;
Adding lots of hardcode to set the visibility of GUI objects;
Reinventing a GUI page manager in JS;
Unintentionally persisting data between pages;
Requiring lots of errorprone case distinctions which are unneeded once relevant GUI objects and code is loaded exclusively.

Add the remember-password checkbox from rP21311 / D822 to the registration page too.

Revert the revert of rP17581 in rP17584 and continue to prefer objects with separate functions per C++ GUI message type,
reducing the nesting of conditionals per function and reveal codeflow by making input and output variables explicit.

Fix oversight in rP20064 by moving the registrationrate string from rP19205 from the "disconnect" to the "error" case.

Differential Revision: https://code.wildfiregames.com/D1568
Few comments by: Vladislav, Imarok, gallaecio

Details

Auditors

Silier
JoshuaJB
wraitii

Committed

elexis

Jun 11 2018, 5:59 PM

Differential Revision

D1568: Split prelobby into multiple pages and require terms acceptance prior to login

Parents

rP21846: Lobby register/login dialog cleanup.

Branches

Unknown

Tags

Unknown

Build Status

Buildable 6241
Build 10363: Post-Commit Build	Jenkins

Event Timeline

elexis committed rP21847: Rewrite the prelobby pages and add the Terms of Service, Terms of Use and the….Jun 11 2018, 5:59 PM

Harbormaster completed building B6241: rP21847: Rewrite the prelobby pages and add the Terms of Service, Terms of Use and the….Jun 11 2018, 6:54 PM

Silier raised a concern with this commit.Jun 12 2018, 8:49 AM

Silier added a subscriber: Silier.

Silier added inline comments.

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/credentials/credentials.xml
11	</object>> -> </object>

This commit now has outstanding concerns.Jun 12 2018, 8:49 AM

why do I have to read terms of services every single time I want to connect? It is annoying and time consuming.

In rP21847#30952, @Angen wrote:

why do I have to read terms of services every single time I want to connect? It is annoying and time consuming.

Because of what the commit message says and because the patch to persist agreement if the terms did not change is not committed yet.

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/credentials/credentials.xml
11	strange, why doesn't this defacto syntax error trigger any errors?

Stan added a subscriber: Stan.Jun 12 2018, 9:36 AM

Stan added inline comments.

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/credentials/credentials.xml
11	Cause it's allowed to have > in XML as long as it's wrapped correctly with CDATA I guess.

elexis added inline comments.Jun 12 2018, 9:46 AM

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/credentials/credentials.xml
11	Why does gameboy get a syntax error on windows but I don't get one on linux? This must be a version thing or a platform bug. https://wildfiregames.com/forum/index.php?/topic/24480-svn21848-error/ ERROR: RelaxNGValidator: Validation error: gui/prelobby/common/credentials/credentials.xml:2: Did not expect text in element object content ERROR: RelaxNGValidator: Validation failed for '(null)' ERROR: CXeromyces: failed to validate XML file gui/prelobby/common/credentials/credentials.xml Yet I can trigger for instance this by adding a syntax error ERROR: RelaxNGValidator: Validation error: gui/prelobby/common/credentials/credentials.xml:4: Did not expect text in element object content

Different version of libxml maybe ?

In rP21847#30952, @Angen wrote:

why do I have to read terms of services every single time I want to connect? It is annoying and time consuming.

(I have accepted the terms and conditions about 200 times when testing the patch)

elexis mentioned this in D1574: Get XML syntax errors not only the first time that file is loaded.Jun 12 2018, 10:54 AM

elexis mentioned this in rP21849: Fix XML syntax error in rP21847 reported by gameboy..Jun 12 2018, 11:19 AM

Fixed by rP21849, blame #5222.

This commit now requires verification by auditors.Jun 12 2018, 11:21 AM

Silier accepted this commit.Jun 12 2018, 11:22 AM

All concerns with this commit have now been addressed.Jun 12 2018, 11:22 AM

elexis mentioned this in D1575: Persist lobby terms agreement checkbox if the terms didn't change.Jun 12 2018, 12:33 PM

elexis mentioned this in D1590: 0 A.D. Empires Ascendant Multiplayer Lobby Privacy Policy.Jun 28 2018, 2:57 PM

There seems to be an issue where we do not always correctly persist the state of the user's acceptance on first-run.
Repro:

Reset to default config
Open lobby, enter incorrect password, accept terms, click login
Observe incorrect password message
Enter correct password
Observe requirement to re-read and accept the terms, even though you already did that during your earlier login attempt

This commit now has outstanding concerns.Jul 16 2018, 4:26 AM

This is not a bug, consider two brothers on the same computer with 2 different accounts, one of them tries to login for the other one, but the one logging in is the one who had to read it and the confirmation that the password is correct is the only way of confirming that the one who read the terms is the one who accepted it.

This commit now requires verification by auditors.Jul 16 2018, 12:28 PM

Okay, seems reasonable; we want the user to verify their identity before accepting the terms. We could ask for the user to re-accept the terms (if they've changed) on another page after the login one to improve the experience. I can look into implementing that (and maybe storing the terms acceptance state per-account on the server) if there are no objections.

All concerns with this commit have now been addressed.Jul 16 2018, 8:18 PM

elexis mentioned this in rP20064: XmppClient cleanup..Sep 1 2018, 3:15 PM

elexis mentioned this in rP21887: Refactor lobby terms and conditions UI from rP21847 / D1568 to a new terms….Sep 14 2018, 5:15 PM

Press continue, click and accept any of the prelobby docs, see that the continue button is enabled again and press it.

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
31	This should check for xmpp client existence.
/ps/trunk/binaries/data/mods/public/gui/prelobby/login/login.js
24	Alternatively, here.

lyv added inline comments.Feb 1 2019, 6:11 PM

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
15	Using this opportunity to mention whether assignment in the while loop is nicer.

In rP21847#32343, @smiley wrote:

Press continue, click and accept any of the prelobby docs, see that the continue button is enabled again and press it.

Seems like the first steps and the observed problem are missing.

I didn't test yet, I suppose a failed registration attempt leaves g_XmppClient initialized, but it should be destroyed after the failure, so that the user can retry then with another nickname?
If that's the case, then the right fix would be to call StopXmppClient or whatever it was instead of disabling the continue button?

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
15	I've never been a fan of assignments within conditions, but given that we our codebase uses JS operators the way they work (for example && and \|\| returning not boolean but one of the condtiions), it seems that could become a new JS fetish too. Should just keep these assignments within conditions simple so that noone will mistake it for a read-only line.

Seems like the first steps and the observed problem are missing.

Not registration, just when logging in. Although, the problem would probably exist then as well. The problem is a second xmpp client being created while the earlier one is logging in. And hence an assertion failure.

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js

Index: binaries/data/mods/public/gui/lobby/lobby.js
===================================================================
--- binaries/data/mods/public/gui/lobby/lobby.js	(revision 21929)
+++ binaries/data/mods/public/gui/lobby/lobby.js	(working copy)
@@ -1292,13 +1292,10 @@
 	updateTimers();
 
 	let updateList = false;
+	let msg;
 
-	while (true)
+	while ((msg = Engine.LobbyGuiPollNewMessage()))
 	{
-		let msg = Engine.LobbyGuiPollNewMessage();
-		if (!msg)
-			break;
-
 		if (!g_NetMessageTypes[msg.type])
 		{
 			warn("Unrecognised message type: " + msg.type);

Is much nicer IMO. But that's just my opinion.

lyv added inline comments.Feb 2 2019, 12:52 PM

/ps/trunk/binaries/data/mods/public/gui/prelobby/common/feedback/feedback.js
15	it seems that could become a new JS fetish too Not really JS specific and it would be valid in C/C++ (among others) too. But yeah, I get where you are coming from.

That let outside of the loop scope is a disadvantage though unless we want to use that variable outside of the loop.

As far as I recall, the C++ g_XmppClient instance should be set if and only if the client has successfully registered an account (and this then nuked) or when having successfully logged in (and thus left this page).
If a registration or login failure occured, the instance should be deleted, so that the user can try again, right?
(Then there's the hypothetical race condition where the user doubleclicks on some XmppClient initialization button twice before it's disabled. But that should be caught already.)

Then there's the hypothetical race condition where the user doubleclicks on some XmppClient initialization button twice before it's disabled. But that should be caught already

Evidently, it is not. That is exactly the problem (at least that’s the gist of it.)

Can just do let msg = too I suppose.

In rP21847#32355, @smiley wrote:

Can just do let msg = too I suppose.

(In the while condition? Is that even legal? I'm currently stuck in c++ bytestream hell otherwise I'd have tested or looked up.)

In rP21847#32355, @smiley wrote:

Then there's the hypothetical race condition where the user doubleclicks on some XmppClient initialization button twice before it's disabled. But that should be caught already

Evidently, it is not. That is exactly the problem (at least that’s the gist of it.)

There totally was a commit that precisely disabled some prelobby confirmation button, did silly me unconsciously revert that nonsilly commit of mine? -.-

(In the while condition? Is that even legal? I'm currently stuck in c++ bytestream hell otherwise I'd have tested or looked up.)

Probably not. ~~(Feeling too meh to test)~~
Edit: Yes, it is not legal.

There totally was a commit...

I don't think this issue is the same.

I guess patches speak louder than comments on an old commit. Although, speaking loud doesn't necessarily mean someone is listening.

rP20053
Can you raise a concern, so that we don't forget about it?

Concern:

this reverted rP20053 by accident, and we should again disable the login/registration button when clicking on them to prevent acting twice, which crashes.

See inline for where I think we need to fix this.

/ps/trunk/binaries/data/mods/public/gui/prelobby/login/login.js
24	From what I understand we need `Engine.GetGUIObjectByName("continue").enabled = false;` here
/ps/trunk/binaries/data/mods/public/gui/prelobby/register/register.js
20	and here

This commit now has outstanding concerns.Jun 25 2019, 2:37 PM

elexis added inline comments.Jun 25 2019, 3:06 PM

/ps/trunk/binaries/data/mods/public/gui/prelobby/login/login.js
24	Also important to add a comment `// Catch double-click` or `// Don't crash on doubleclick`, otherwise the line will be removed yet again in some future cleanup.
/ps/trunk/binaries/data/mods/public/gui/prelobby/register/register.js
25	Also, in `JSI_Lobby::StartXmppClient` and `ConnectXmppClient`, the `ENSURE(!g_XmppClient);` should be changed to a JS error, because one doesn't need a C++ stacktrace but a JS one to debug the according error.