TagActions.remove must return a single data type

[atodorov]

This method either returns a string or a tuple of (bool, object) which makes me wonder is the return value even used ! This obviously needs to be examined and rectified.

Update: while at it the method uses a .filter()[0] expression with an except IndexError to figure out if an object exists. Obviously this can be written more organically as .get() except ObjectDoesNotExist.

[asankov]

After investigating the issue it turned out that TagActions is an internal class for the tag method. The only place where the remove method could be called is in the following snippet

q_action = request.GET.get('a')
if q_action:
    tag_actions = TagActions(obj=obj, tag=q_tag)
    func = getattr(tag_actions, q_action)
    response = func()
    if not response[0]:
        return HttpResponse(json.dumps({'response': response, 'rc': 1}))`. 

Here func can be either TagActions.add() or TagActions.remove(). Both functions would return either a string (error message), or a (True, string) tuple (when the function has completed successfully). That means that if not response[0] would always be false, and the return on the next line is unreachable. However, the response object is used few lines below

    if request.GET.get(f) == serialized:
        return HttpResponse(
            # FIXME: this line of code depends on the existence of a
            # argument in query string. So, if a does not appear in the
            # query string, error will happen here
            serializers.serialize(request.GET[t], response[1])
        )

The author of the code has left a comment, that a possible error could occur here if request.GET.get('a') does not exist. I think we could think of a way to fix this, and in the meantime come up with a solution about the return type of both TagActions.remove and TagActions.add. Another thing is that should any of these function return string, then response[1] would be just a character, which we would be sending as response, and I'm not sure that this is a desired outcome.

Possible solution would be the return time of the TagActions.add()/remove() to be a tuple where the first value is a boolean, showing whether the function has completed successfully, and the second to be either a string or an object (error message as a string, when failure. the object when success). This would allow us to send a response based on the successful completed of the function.

[atodorov]

That means that if not response[0] would always be false, and the return on the next line is unreachable.

This is correct (mostly). The if condition will evaluate to True if the first letter of the error message is empty string or we return a tuple with first element False, which is not the case in practice.

A quick fix would be to return (False, "Error message") and use response[1] in case of error. You have to fix this for both the remove and add methods.

However the entire view function doesn't look right. All of the remove_tag methods will not raise exceptions so catching anything is pointless, not sure about the rest.

@asankov if you want to continue working on this I suggest first adding some tests and then applying the quick fix before we can discuss anything further.