Open-Assistant
Open-Assistant copied to clipboard
LAION-AI
fix: upsert user reaction instead of inserting
Hello,
I played with the end to end demo and saw a bug in the /grading/grade-output page.
If I submit a rating twice the second time I submit it I get the following error in the backend:
sqlalchemy.exc.IntegrityError: (psycopg2.errors.UniqueViolation) duplicate key value violates unique constraint "post_reaction_pkey"
DETAIL: Key (post_id, person_id)=(702250a7-0f3b-4469-9e86-ab7de669e832, e5602260-05cf-4a4b-8926-7fe53f09a6dc) already exists.
Indeed we are trying to do an insert instead of an update. I understand that in the end a user may not be supposed to submit a rating twice on the website (page updated once submited).
But depending on the frontend it can be nice to have an update instead (e.g. discord)
(In the discord example when I try to "update" the rating it failed)
I don't know if this is also nice to have for insert_post, let me know if you want me to also change it.
Thank you for catching this! Yes, that is a problem i've noticed as well.
Did you see this?
We have to take notice that updates will invalidate previous entries in the event-journal, e.g. it becomes harder to incrementally compute the item scores .. e.g. we need to query all effective post_reactions and can't rely on the journal data.
the way it looks to me now, even if an update happens, the log_ranking of the journal is still called, e.g. the old reaction log would still be in the journal. I don't have a good overview over the codebase, do you think I'm overlooking something here?
to clarify: if we do upserts, then the journal entries would have to be corrected/updated/replaced accordingly. in general, I'm fine yelling at the user if they submit the rating twice. yes we'll get some mistakes where users change their minds later, but that's life.
I added a new commit to remove the entry in the journal table when we update an existing user reaction.
However, I could understand if you have concerns about implementing this change and prefer to stick with the current behaviour of not allowing upserts. If this is the case I'll just close this PR.