approve: handle usernames not in session #126
Conversation
ocflib/account/submission.py
Outdated
| request = get_remove_row_by_user_name(user_name).to_request() | ||
| stored_request = get_remove_row_by_user_name(user_name) | ||
| if not stored_request: | ||
| return |
There was a problem hiding this comment.
should this return an error so that the caller knows about this? i think it should
There was a problem hiding this comment.
0 and 1 or an error string?
There was a problem hiding this comment.
looking at this from an interface perspective and ignoring celery and all the other details, i'd expect that if i call a function approve_request(username) and there's no request to be approved with that username, that it raises an exception of some kind, since it wasn't able to approve any request.
i actually think that raising an exception here is probably the right approach, but i get that it's a bit inconvenient because of the way we do error reporting from the create workers (i assume the point of this branch is to avoid the ocflib error emails?).
turns out we actually already exempt ValueErrors from the reporting "middleware", since they usually indicate caller error: https://github.com/ocf/create/blob/f937a2b88984a2b2ffd282421dbf4e8e429c9a4a/create/tasks.py#L60-L69
maybe we should just raise a ValueError here? it won't generate an ocflib report, and callers (ocfweb or ircbot or whatever) would be able to catch and handle the exception if they want to. if they don't catch the exception, it would (ideally) trigger a crash in ocfweb/ircbot/etc, which is appropriate since this is sort of a caller error.
(i don't think it will actually trigger an error in ircbot, since i don't think it waits to see the result of the call /shrug)
does that seem reasonable? it's possible that my expectations for how this function should behave are inconsistent with other peoples'.
There was a problem hiding this comment.
I do think raising an exception makes more sense, given that is really an error from the point of view of the function or any caller. It seems to me that relying on ValueErrors to just not be reported is counter-intuitive though since I don't think anyone would expect that they are just ignored but all other errors are reported.
Maybe longer-term we should have a custom error type that isn't reported for this kind of thing so that errors can be raised that are expected to be handled by callers instead of sending ocflib reports?
There was a problem hiding this comment.
Yeah, I was thinking about an exception when I first made this PR but it felt weird that, in order to stop an exception, we raise one somewhere else. After fleshing out the options it does make the most sense though.
There was a problem hiding this comment.
This PR does feel kinda useless for the time being. The caller is gonna catch a ValueError as opposed to an AttributeError. That said, if we do go the custom exception route in the future it will make a lot more sense...
ocflib/account/submission.py
Outdated
| create_account.delay(request) | ||
| dispatch_event('ocflib.account_approved', request=request.to_dict()) | ||
|
|
||
| @celery_app.task | ||
| def reject_request(user_name): | ||
| stored_request = get_remove_row_by_user_name(user_name) | ||
| if not stored_request: | ||
| return |
kkuehlz
force-pushed
the
nonexistent_submission
branch
2 times, most recently
from
d3b0417 to
6c0ee4c
Compare
|
@chriskuehl @jvperrin fixed and force pushed. the first build failed i think because jenkins builds in tmpfs and supernova |
ocflib/account/submission.py
Outdated
| request = get_remove_row_by_user_name(user_name).to_request() | ||
| stored_request = get_remove_row_by_user_name(user_name) | ||
| if not stored_request: | ||
| raise ValueError('User not in session') |
There was a problem hiding this comment.
I'd say something like "No pending request for user" or something, I don't think the current message is really descriptive of what the problem actually is (what session?).
ocflib/account/submission.py
Outdated
| create_account.delay(request) | ||
| dispatch_event('ocflib.account_approved', request=request.to_dict()) | ||
|
|
||
| @celery_app.task | ||
| def reject_request(user_name): | ||
| stored_request = get_remove_row_by_user_name(user_name) | ||
| if not stored_request: | ||
| raise ValueError('User not in session') |
both of the added tests will fail prior to this patch. users of the approve_request and reject_request functions should catch a ValueError. fixes #81
kkuehlz
force-pushed
the
nonexistent_submission
branch
from
6c0ee4c to
ef495d5
Compare
both of the added tests will fail prior to this patch.
fixes #81