Page MenuHomeWildfire Games

Display a nice error message when trying to register too many lobby accounts
ClosedPublic

Authored by Imarok on Jan 23 2017, 11:17 PM.

Details

Summary

We should display a nice error message when someone tries to register too many lobby accounts within a short period of time, as nobody knows about this limitation and wonders why the registration doesn't work.

Test Plan

Try to register > 1 accounts within one hour.

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

Imarok created this revision.Jan 23 2017, 11:17 PM
Owners added a subscriber: Restricted Owners Package.Jan 23 2017, 11:17 PM
Imarok updated the Trac tickets for this revision.Jan 23 2017, 11:18 PM
Vulcan added a subscriber: Vulcan.Jan 24 2017, 12:08 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/233/ for more details.

Shouldn't what is the actual value of the RegistrationResult? Might be better to add a case for that and return a better error. (Or patch the whole path if there isn't such a thing yet.)

scythetwirler added inline comments.
binaries/data/mods/public/gui/lobby/prelobby.js
166 ↗(On Diff #317)

Removing the comma would make this sentence grammatically correct.

elexis added a subscriber: elexis.Jan 25 2017, 4:57 PM
In D87#2907, @leper wrote:

Shouldn't what is the actual value of the RegistrationResult? Might be better to add a case for that and return a better error. (Or patch the whole path if there isn't such a thing yet.)

+1 for catching the error explicitly and not adding vague strings.
See 3771#comment:14 for an analysis on what would be needed to make this happen.
Long story short, we would have to start shipping a patched version of the gloox, as even the latest stable of gloox drops the case check and returns the default unknown case.
The patch is easy to write, shipping gloox would be harder.

In D87#3171, @elexis wrote:

Long story short, we would have to start shipping a patched version of the gloox, as even the latest stable of gloox drops the case check and returns the default unknown case.
The patch is easy to write, shipping gloox would be harder.

Submit the patch upstream (though upstream might want copyright assignment or similar, since the author does sell that commercially), or just ask nicely about getting that fixed?

Once that is upstream support the case in our code (since that enum already exists we do not even need to add an ifdef around it), and update gloox for win32 then wait for distros to start shipping recent versions and consider this done.

I suggest adding this workaround anyway, as it could need some time, until the patch gets upstream.

Sure, I'm bothered too by the weekly repetition of the question in the lobby.
But we shouldn't forget to take care of informing the gloox author(s?) beforehand and potentially suggest a patch, since that one doesn't seem hard to write.

Notice the text field is optional, as seen in section 9.3.2. of https://www.ietf.org/rfc/rfc3920.txt

elexis requested changes to this revision.Jan 28 2017, 5:28 PM
elexis added a reviewer: elexis.
elexis edited edge metadata.Jan 29 2017, 6:07 AM

Since trac is dead again, posting this here:

Gloox patch: P10
0 A.D. patch: P11

In order to test:

  1. Compile patched gloox:
    1. Uninstall gloox from your system, to ensure it's not being used?
    2. Download the most recent gloox version: https://camaya.net/gloox/download/
    3. Apply the gloox patch
    4. Compile it using ./configure and make
  1. Compile patched 0 A.D. with the patched gloox:
    1. Place a link from the built gloox shared object file libgloox.so.15 in gloox/src/.libs/ to binaries/system/
    2. Patch 0 A.D.
    3. Run clean-workspaces.sh unfortunately and compile as usually

The gloox patch was proposed at https://bugs.camaya.net/ticket/?id=267

It seems we won't be able to merge the patch unless requiring the updated gloox version

elexis accepted this revision.Feb 7 2017, 2:04 PM

Considering that it will take months if not years until the patch is merged upstream and most distributions include that gloox version...
Don't forget the comma, perhaps IP -> IP address and leave the ticket open

This revision is now accepted and ready to land.Feb 7 2017, 2:04 PM
Imarok updated this revision to Diff 474.Feb 7 2017, 4:02 PM

Comma

This revision was automatically updated to reflect the committed changes.
Vulcan added a comment.Feb 7 2017, 4:46 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/327/ for more details.

Patch was merged upstream, so we can now add an ifdef and a proper string case while keeping the workaround until we bump the minimum version required of gloox to compile 0ad with.

In D87#6025, @elexis wrote:

Patch was merged upstream, so we can now add an ifdef and a proper string case while keeping the workaround until we bump the minimum version required of gloox to compile 0ad with.

Kill the workaround, update the win32 version, the version we use for OSX, and let outdated linux distros get what they deserve.