CODEWITHSUNDEEP
pythondjango
Ahmed Wagdi
Ahmed Wagdi

Reputation: 4371

Django - Cannot save new entry to database using view function

here is the view function I am using

views.py

def favorite_item (request, pk):
    favitem = get_object_or_404(Item, pk=pk)
    userfav = Favorite.objects.filter(user=request.user)
    for items in userfav:
        if items.item == favitem:
            items.delete()
        else:
            new_entry = Favorite(item=favitem, user=request.user)
            new_entry.save()

    return HttpResponseRedirect(request.META.get('HTTP_REFERER'))

the part of deleting already exists row working perfectly :

   if items.item == favitem:
        items.delete()

but i think the problem is where i try to add a new row , just returns nothing not even raising an error !

else:
    new_entry = Favorite(item=favitem, user=request.user)
    new_entry.save()

Upvotes: 1

Views: 133

Answers (1)

willeM_ Van Onsem
willeM_ Van Onsem

Reputation: 476557

The logic is probably off. favitem is probably the item the user wants to favorite/unfavorite. Furthermore userfav is the list of favorite items of that user.

if-else clauses in for loops

But then you start iterating over that list. In case the Favorite is the item to favorite you remove it (this is probably correct). But in case the item in the favorite list of the user is not the favorite of that user, you add another Favorite object, so now there are two Favorites linking such element with the user.

Fixing the logic

You probably want to execute the else part only in case the if part was never (i.e. in no loop) was executed. We can do that with a for-else construct:

for items in userfav:
    if items.item == favitem:
        items.delete()
        break
else:
    new_entry = Favorite(item=favitem, user=request.user)

So note that here the else is at the same level as the for loop. And thebreakis crucial, since this will make sure theelse` part is not fired.

Optimizing the procedure

But we make it too hard. We can simply let Django perform the filtering, and remove the favorites. We can filter(..) the queryset on the item as well, and remove these entries. In case no entry was removed, we can then add such Favorite entry:

def favorite_item (request, pk):
    favitem = get_object_or_404(Item, pk=pk)
    num, __ = Favorite.objects.filter(user=request.user, item=favitem).delete()
    if not num:
        new_entry = Favorite.objects.create(item=favitem, user=request.user)

    return HttpResponseRedirect(request.META.get('HTTP_REFERER'))

So here num will contain the number of Favorite entries removed. In case that is not zero, we removed one or more Favorites for which the user is request.user, and item is our favitem.

In case num is not zero, we then .create(..) a new Favorite.

Constraining the Favorite model

You also better constrain your Favorite model. Right now, it looks like a user can make the same Item their Favorite multiple times. You better use unique_together for this, to prevent that:

class Favorite(models.Model):

    item = ForeignKey(Item, on_delete=models.CASCADE)
    user = ForeignKey(User, on_delete=models.CASCADE)

    class Meta:
        unique_together = (('item', 'user'), )

Of course I here make some assumptions on how the Favorite model looks. The important part is however the unique_together.

In case we have added such constraint, we know that for a .filter(user=..., item=...) there will always be at most one such element. So in that case the logic is even simpler:

def favorite_item (request, pk):
    favitem = get_object_or_404(Item, pk=pk)
    try:
        Favorite.objects.get(user=request.user, item=favitem).delete()
    except Favorite.DoesNotExist:
        new_entry = Favorite.objects.create(item=favitem, user=request.user)

    return HttpResponseRedirect(request.META.get('HTTP_REFERER'))

Upvotes: 1

Related Questions