gmg user commands don't fail gracefully for non existant users

[ayleph]

Attempt to delete a non-existent user using bin/gmg deleteuser and you'll get a Traceback similar to below.

$ bin/gmg deleteuser idontexist
Traceback (most recent call last):
  File "bin/gmg", line 9, in <module>
    load_entry_point('mediagoblin==0.7.1.dev', 'console_scripts', 'gmg')()
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/__init__.py", line 124, in main_cli
    args.func(args)
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/users.py", line 132, in deleteuser
    username=unicode(args.username.lower())).one()
  File "build/bdist.linux-x86_64/egg/sqlalchemy/orm/query.py", line 2329, in one
sqlalchemy.orm.exc.NoResultFound: No row was found for one()

Same problem exists for other commands in users.py.

[ayleph]

Elrond suggested the following change, which tested out fine for existing and non-existing users on my system.

mediagoblin/gmg_commands/users.py:

def deleteuser(args):
    commands_util.setup_app(args)

    db = mg_globals.database

    user = db.User.query.filter_by(
        username=unicode(args.username.lower())).one()

change to

def deleteuser(args):
    commands_util.setup_app(args)

    db = mg_globals.database

    user = db.User.query.filter_by(
        username=unicode(args.username.lower())).first()

[ayleph]

Fix pushed to ​https://gitorious.org/mediagoblin/aylephs-mediagoblin/commit/f0e795df82596e476d9f116d58012e563bb064a4

[ayleph]

Pushed to master.

​https://gitorious.org/mediagoblin/mediagoblin/commit/f0e795df82596e476d9f116d58012e563bb064a4

[Elrond]

We just realized, that users.py is affected globally.

Notes:

1. Wait until py3 merge is over!
2. Instead of doing unicode(...) (or six.text_type) in the db query, try doing a type=six.text_type in the .add_argument of the parser setup, so that the parser already does the typing for us.

[Jessica Tallon]

I have used the type=sex.text_type as suggested above in the parser setup. The rest of this bug looks to be resolved though, current master:

(mediagoblin)[jessica@macbook mediagoblin]$ ./bin/gmg deleteuser idontexist
The user idontexist doesn't exist

As far as I can see that makes this issue closed as everything reported has been fixed. The fix I pushed is in ​a02de38. If I've missed anything feel free to re-open this issue or open a new issue with the specific issue.

[ayleph]

Originally I opened the ticket when someone pointed out that deleteuser was failing for a user that didn't exist. Elrond fixed that and merged it. Later we realized it applied to other commands as well, so we re-opened the ticket. This is still the case. The fix needs to be applied to the other commands.

For example, changepw:

$ bin/gmg changepw idontexist mynewpassword
** Message: pygobject_register_sinkfunc is deprecated (GstObject)
Traceback (most recent call last):
  File "bin/gmg", line 9, in <module>
    load_entry_point('mediagoblin==0.7.2.dev', 'console_scripts', 'gmg')()
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/__init__.py", line 156, in main_cli
    args.func(args)
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/users.py", line 115, in changepw
    username=six.text_type(args.username.lower())).one()
  File "build/bdist.linux-x86_64/egg/sqlalchemy/orm/query.py", line 2329, in one
sqlalchemy.orm.exc.NoResultFound: No row was found for one()

And makeadmin:

$ bin/gmg makeadmin idontexist
** Message: pygobject_register_sinkfunc is deprecated (GstObject)
Traceback (most recent call last):
  File "bin/gmg", line 9, in <module>
    load_entry_point('mediagoblin==0.7.2.dev', 'console_scripts', 'gmg')()
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/__init__.py", line 156, in main_cli
    args.func(args)
  File "/path/to/mediagoblin/mediagoblin/gmg_commands/users.py", line 88, in makeadmin
    username=six.text_type(args.username.lower())).one()
  File "build/bdist.linux-x86_64/egg/sqlalchemy/orm/query.py", line 2329, in one
sqlalchemy.orm.exc.NoResultFound: No row was found for one()

[Christopher Allan Webber]

Worth fixing, but can be bumped to next release, not a blocker for 0.8.0

[Ben Sturmfels]

I'm attaching a patch to changepw and makeadmin to prevent these unhandled exceptions when the user doesn't exist.

Regards,
Ben

[Ben Sturmfels]

I should mention that I've manually tested both valid and invalid users for changepw and makeadmin.

[ayleph]

Looks good to me, at a glance. CC'ing berker to consider pushing to master.

[berkerpeksag]

I couldn't apply the patch cleanly. Could you please fix merge conflicts?

+        print(u'The user %s doesn\'t exist.' % args.username)

Can you change %s to %r? The user 'foo' doesn't exist. looks better to me. Same comment for other usages of %s.

+        type=six.text_type)

Clever! :)

Thanks!

[ayleph]

I couldn't apply the patch cleanly. Could you please fix merge conflicts?

Oh, I see now that tsyesika's ​changes from earlier today modified this file after Sturm's patch was submitted. How exciting!

[Ben Sturmfels]

Thanks for the review Berker. I've resolved the conflict in the attached patch.

[Ben Sturmfels]

Conflicts resolved.

[Christopher Allan Webber]

The patch looks good, and it didn't break any patches that aren't already broken in master (I'd better look into those!)

Great work Sturm, thanks for the patch! And thanks for your review berker, as well as your contributions on this one ayleph!

Sometimes things take a while, but they make it... :)