Signbank / Global-signbank

An online sign dictionary and sign database management system for research purposes. Developed originally by Steve Cassidy/ This repo is a fork for the Dutch version, previously called 'NGT-Signbank'.
http://signbank.cls.ru.nl
BSD 3-Clause "New" or "Revised" License
19 stars 12 forks source link

Error in changing gloss #1330

Closed uklomp closed 3 weeks ago

uklomp commented 1 month ago

When editing a gloss on Signbank, I click 'edit' and write the new gloss, and save it. It often does not save in one time, I often need multiple attempts. Usually after 2 or 3 times, or refreshing the page, it saves, but today it doesn't seem to work - at least not for the English gloss for REPAREREN-A (in Dutch). I get this error:

image
susanodd commented 1 month ago

Is this for changing the Annotation or the Lemma ? It looks like you are editing the Annotation (English) field? Is that correct? And it won't allow to save?

FYI, when changes are made to Lemmas or Annotations, the video files are moved / renamed. But if there is no video yet, that should not be an issue.

susanodd commented 1 month ago

We will need to look at the logs.

susanodd commented 1 month ago

@uklomp what do you want the English to be? It's FIX-B right now. Is that what it's supposed to be?

ngt-repareren-a-fix-b

susanodd commented 1 month ago

From the log:


Internal Server Error: /dictionary/update/gloss/46332
django.core.exceptions.ValidationError: ["The annotation idgloss translation text 'REPAIR-A' is not unique within dataset 'NGT' for gloss '46332'."]

That happened 3 times.

I'll see why it's not giving a message to the user.

https://github.com/Signbank/Global-signbank/blob/5db93d5c71fda62f46a5ec4b37ba8421b499dc35/signbank/dictionary/models.py#L3444-L3452

Unfortunately, this "message" is inside of the save method of the AnnotationIdglossTranslation model. This does not get passed back to the template. (It ought to be in the view or in the form.)

susanodd commented 1 month ago

I've got it fixed (locally):

error-message-annotation

susanodd commented 1 month ago

[CODE, related to #1331] User feedback is going to be stuck in English for a while. The translations need to be worked out how to apply them to exception messages that are passed around internally. Possibly the interface language needs to be added to more gloss model methods.

susanodd commented 1 month ago

@uklomp This is live on signbank.

uklomp commented 1 month ago

Ok I get your first questions: Yes, it was about the English annotation ID, and it didn't allow me to save. The other messages I don't quite get but I think you wanted me to try it again. So what I wanted to do is change FIX-B to REPAIR-A and then it pretended to be saving, but then the field became empty, so no English annotation gloss at all anymore?? And then I refreshed and the old text FIX-B is back...

susanodd commented 1 month ago

Ok I get your first questions: Yes, it was about the English annotation ID, and it didn't allow me to save. The other messages I don't quite get but I think you wanted me to try it again. So what I wanted to do is change FIX-B to REPAIR-A and then it pretended to be saving, but then the field became empty, so no English annotation gloss at all anymore?? And then I refreshed and the old text FIX-B is back...

I repaired it so it should work for you now. You will get feedback about why it's not possible if it fails.

Yes, it pretended to be saving. But the "save" resulted in a database constraint violation. That was visible in the error logs on the server, but the code did not say anything about this to the user. Now it does.

[CODE] There were internal error messages, but they were not sent back to the user. I fixed it so you will see them. [DATABASE CONSTRAINTS] The error happens when the desired change would result in a constraint violation on the annotations.

uklomp commented 1 month ago

No, not working yet! Als no error. But apart from not receiving an error message, it should also just work haha, so there are now two problems:

susanodd commented 1 month ago

No, not working yet! Als no error. But apart from not receiving an error message, it should also just work haha, so there are now two problems:

  • the error message
  • the fact that it's not changing into what it should be

I deployed the code so it should be working. Can you reload the page several times, just in case? It displays the feedback in a "messages" element in the Gloss view. It could be behind other code or something. It's a banner at the top of the page, under the signbank header.

It won't change and you can't change it if what you want to do violates a database constraint. You are trying to do an operation that violates a database constraint. I saw this in the error logs on the server. So now that error ought to be visible.

(It's not a code "error", it's a "data" error.)

We can do this live at the next meeting and you can show us what is happening if it still doesn't show error messages.

You could try to do it in the Batch Edit. That also has error reporting. You'd need to do a query first, then go to the page.

uklomp commented 1 month ago

No, there's no error message. It should be on Signbank yes? Not on the susan one or the dev on?

And I get your point, but at the same time, as a user, this is non-informative to me. My request is, how can we solve it? Because I should be able to change it.

susanodd commented 1 month ago

No, there's no error message. It should be on Signbank yes? Not on the susan one or the dev on?

And I get your point, but at the same time, as a user, this is non-informative to me. My request is, how can we solve it? Because I should be able to change it.

Yes, it's on Signbank. The reason you are not seeing the error message, the only thing I can think of is that the "html element" is hidden behind something else.

I can tell you how to fix it. You need to change the Annotation of whatever "other gloss" has the same annotation that you want to use. The annotations are supposed to be unique.

If you look at the screenshot above where the feedback is visible, it explains what is wrong. The annotation is not unique in the dataset. (The error message text was written by @vanlummelhuizen years ago when datasets were introduced.)

You could temporarily change the other gloss and then change this one, and then change the other to something else ("to swap the annotations") (Such an operation cannot be done automatically. The user needs to do this.)

susanodd commented 1 month ago

I will see if we can add hints or something somewhere for resolving database constraints

uklomp commented 1 month ago

Ok, I get it now, but then how come I don't find the "other gloss" with the same annotation? I mean, it should appear when I search for it, right? It didn't seem to exist

Woseseltops commented 1 month ago

For completeness, when you get an error message, one of these things is happening:

  1. Something server side got into an illegal state
  2. The user did something that is not possible/allowed
  3. The error message was created incorrectly, and nothing is wrong

The question now is which one applies here :)

susanodd commented 1 month ago

Ok, I get it now, but then how come I don't find the "other gloss" with the same annotation? I mean, it should appear when I search for it, right? It didn't seem to exist

Yes, you should be able to find the other gloss that it conflicts with. If that's not the case then that would be a different problem. So (3) in @Woseseltops 's list above. We can ask @vanlummelhuizen since the ValidationError that's raised is from him.

I think this is it (selected dataset NGT):

https://signbank.cls.ru.nl/dictionary/gloss/48346/

I searched on "repair" in the Gloss Search in the menu bar. That search is multi-lingual. You need to click on the magnifying glass icon to search.

vanlummelhuizen commented 1 month ago

@uklomp REPAIR-A is here: https://signbank.cls.ru.nl/dictionary/gloss/48346/


@susanodd If I use the dev-tools in my browser (by hitting F12) I can see in the raw html of the response that the message is there:

    <div class="alert alert-danger alert-dismissable">
        <button type="button" class="close" data-dismiss="alert" aria-hidden="true">&#215;</button>
        The annotation idgloss translation text &#x27;REPAIR-A&#x27; is not unique within dataset &#x27;NGT&#x27; for gloss &#x27;46332&#x27;.
    </div>

However, the html of the current page does not show it. Also, the console shows many errors. Finally, in the first comment from @uklomp there is a browser alert showing "There was an error processing this change: " and some incomprehensible html code.

This all leads to the conclusion that after hitting OK there is a so called Ajax-request that should return a short message (perhaps JSON) that is processed by the Javascript that initialized the Ajax-request. There is no complete page-reload that would show the message shown above.

So, the lines

https://github.com/Signbank/Global-signbank/blob/6242cc47de593c3debcd78da7a2f2a25f97f0f62/signbank/dictionary/update.py#L1170-L1176

should not return a HttpResponseRedirect but something that https://github.com/Signbank/Global-signbank/blob/6242cc47de593c3debcd78da7a2f2a25f97f0f62/media/js/gloss_edit.js#L369-L380 could interpret and show in a browser alert.


:bulb: Tips:

susanodd commented 1 month ago

That's pretty weird because other code that returns the messages works okay. And all the code works as expected on PyCharm. Maybe it's not supposed to be a redirect. Newer code I wrote does not use the "edtiable" but json response instead. The update gloss code is very old. There are probably other places that work just fine showing errors if it's on PyCharm runserver, but maybe not on Apache. No idea.

susanodd commented 1 month ago

I suspect there is not supposed to be a redirect, but just a response. [update: other branches in the same update method all return response, not redirect] Most of the already existing code for gloss update just does nothing if there are errors. (We make assumptions that if choices are selected, the values have been obtained from the choice lists. If the choice does not exist, then it either does nothing or sets it to '-'.) The code that runs after the editable call happens processes a tab-separated string of results. But does not account for errors, because the operations just do nothing if there was an error.

(So basically all of the "update gloss" code needs to be modified to include error feedback that works on Apache, not just PyCharm.)

susanodd commented 1 month ago

Modified as so:

error-constraint-as-alert

Messages are more elegant but that requires extensive code updates.

susanodd commented 1 month ago

Hmmm. My GitHub won't let me view my own screenshot (Ubuntu). It says it's a private user image. There is a screenshot above but it may disappear.....

susanodd commented 1 month ago

The alert message feedback has been deployed on signbank.