patchFreeciv - Patches: patch #3743, /delegate tidy-up

 
 
Show feedback again

patch #3743: /delegate tidy-up

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sat 23 Feb 2013 11:05:52 PM UTC  
 
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Jacob Nevins <jtn>Open/Closed: Closed
Planned Release: 2.4.0,2.5.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 03 Mar 2014 12:52:44 AM UTC, comment #11:

For reference, the new ticket is here:
https://gna.org/bugs/index.php?21744

Davide Baldini <davide_at_debian>
Mon 03 Mar 2014 12:46:23 AM UTC, comment #10:

Sounds reasonable without looking in detail; please raise a new ticket for it (this one has been closed for a year, so we'll just lose the comment otherwise).

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 03 Mar 2014 12:39:44 AM UTC, comment #9:

The current implementation of delegations is case-sensitive with regard to usernames. That is, if user "A" delegates his nation to user "B", "b" wouldn't be able to take the delegation from "A".

In Greatturn, games last for months and usernames can be somewhat long and complicated; sometimes users log-in typing their nicks w/o paying attention to case, and then they wonder why delegations stop working.

AFAIK, the whole structure of Freeciv is built to be case insensitive for usernames, so this patch, which comes from Longturn, is somewhat inconsistent. Moreover, Longturn solves this issue - for its sole benefit - by using the following custom code to internally handle delegations in interactions with DB:

I think this patch should be made consistent with the rest of Freeciv, by treating usernames case-insensitively.

Davide Baldini <davide_at_debian>
Fri 01 Mar 2013 10:51:10 AM UTC, SVN revision 22473:

Remove some (disabled) debug code accidentally committed in the previous
fix for gna patch #3743 (r22459).

(Browse SVN revision 22473)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Fri 01 Mar 2013 10:50:36 AM UTC, SVN revision 22472:

Remove some (disabled) debug code accidentally committed in the previous
fix for gna patch #3743 (r22458).

(Browse SVN revision 22472)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 10:37:34 PM UTC, comment #6:

...now of course is the time I notice I left some debugging crap in there, fortunately #if 0'd out.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 10:35:39 PM UTC, comment #5:

No-one screamed, so in it goes.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 10:35:10 PM UTC, SVN revision 22459:

Various cleanups to /delegate.
- admin can now forcibly '/delegate cancel' an active delegation
- player X observer -> player X delegate works better
- generally tighten up checks, particularly when an admin is changing
another player's delegation
- mark strings for i18n
- many other message / help / textual improvements

See gna patch #3743.

(Browse SVN revision 22459)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 10:25:47 PM UTC, SVN revision 22458:

Various cleanups to /delegate.
- admin can now forcibly '/delegate cancel' an active delegation
- player X observer -> player X delegate works better
- generally tighten up checks, particularly when an admin is changing
another player's delegation
- mark strings for i18n
- many other message / help / textual improvements

See gna patch #3743.

(Browse SVN revision 22458)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 25 Feb 2013 01:50:04 AM UTC, comment #2:

> Given that it implements its own access control

...perhaps /delegate should be ALLOW_INFO? I haven't done this.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 25 Feb 2013 01:49:19 AM UTC, comment #1:

Attached a patch. As well as the previous, some more specific issues I know it to fix:

  • a delegate going from observing player X to controlling the same player was not handled (it was spuriously refused)
    • (In general, observers will not by default be able to be delegates, as they have 'info' level but /delegate is ALLOW_BASIC. But delegates who are observers seems a reasonable use case to me. Given that it implements its own access control
  • '/delegate cancel <player>' by an admin while the delegation was active was not handled and would crash
    • It's now handled by forcibly detaching the delegate. This means that an admin should be able to untangle any set of delegations users have set up, which is good IMO as some commands are prevented even for admins with delegations active.
    • (I considered also allowing admins to do third-party '/delegate restore <user>' and allowing '/delegate to' to disconnect the current delegate too, but decided I'd spent long enough polishing this already.)
  • several strings weren't marked for i18n

That's not all the changes though; in general, the checks have been tightened up and should be self-consistent.

It would be good if someone who's used delegate functionality before could review/test this; I've reverse-engineered how it's supposed to work, and what I've ended up with seems useful to me, but it's possible that I've missed the point or some important way in which it is used in practice.

(file #17331)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 23 Feb 2013 11:05:52 PM UTC, original submission:

It started as the usual batch of string changes and kind of ballooned. So far I have:

  • much expanded help for "/delegate"
  • add "/list delegations" to help
  • remove redundant check (per bug #20525)
  • more comprehensive delegation messages when user connects
    • fix bug where info about connections delegated to you not displayed if you were unattached or global observer
  • fix inaccurate messages if delegate user not connected to anything (not even global observer) (this case seems neglected, per bug #20490)
Jacob Nevins <jtn>
Project AdministratorIn charge of this item.

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #17371:  trunk-S2_4-delegate-rework-rm-debug.patch added by jtn (898B - text/x-patch - trunk/S2_4 r22459: remove debug accidentally left in previous patch)
file #17331:  trunk-S2_4-delegate-rework.patch added by jtn (42kB - text/x-patch - trunk/S2_4 r22431)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by davide_at_debian (Posted a comment)
  • -unavailable- added by jtn (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 01 Mar 2013 10:51:34 AM UTCjtnStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Wed 27 Feb 2013 10:43:28 PM UTCjtnStatusIn Progress=>Ready For Test
    Wed 27 Feb 2013 10:43:25 PM UTCjtnAttached File-=>Added trunk-S2_4-delegate-rework-rm-debug.patch, #17371
    Wed 27 Feb 2013 10:37:34 PM UTCjtnStatusDone=>In Progress
      Open/ClosedClosed=>Open
    Wed 27 Feb 2013 10:35:39 PM UTCjtnStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Mon 25 Feb 2013 01:49:19 AM UTCjtnAttached File-=>Added trunk-S2_4-delegate-rework.patch, #17331
      StatusIn Progress=>Ready For Test
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup