Batch Edit

[susanodd]

This is on signbank-test UNDER THE ANALYSIS MENU Analysis -> Batch Edit Analyse -> Batchbewerking

• The toggle edits for handedness, strong hand, weak hand, location are implemented.
• Updates to Lemma, Annotation and Senses fields can be saved.
• if the default annotation is updated, this is reflected in the hyperlink in the details area as well as the scrollbar.
• New (empty) senses can be created
• The Similar Glosses shows matches with the phonology fields (these can be turned off and on with the buttons right above, this allows searching on partial phonology for the specific gloss (there are no videos on signbank-test so this will be cooler on the real server)
• All changes are saved to the Gloss Revision History.

[susanodd]

For Senses, I didn't do anything yet if the user "empties" both language fields. If the user empties one of the languages for the sense, it correctly removes the sense translation (keywords) for that language. But if the user empties both languages, then what? (In Gloss Edit and the Keywords Mapping, the senses are renumbered because one sense has "disappeared". That is kind of too heavy an operation for batch edit to start renumbering senses!) In Batch Edit any Sense # with empty translations remain.

[susanodd]

This branch is on signbank-test now.

[susanodd]

@vanlummelhuizen I have a specific permissions question related to this. (I didn't include you as a reviewer, since this is lots of code in this branch. But specific to permissions, since you're working on that.)

SHALL I JUST GO AHEAD AND CHANGE THIS? I'll remove the checks that are likely unnecessary.

The permissions for the "mini updates" are checked in e.g., here

https://github.com/Signbank/Global-signbank/blob/eddec4b11447b20323453b387ff0aa3f9f2cb982/signbank/dictionary/update.py#L3525-L3536

and here in the called function (btw is the decorator and specific check redundant?)

https://github.com/Signbank/Global-signbank/blob/eddec4b11447b20323453b387ff0aa3f9f2cb982/signbank/dictionary/update_glosses.py#L216-L222

The above ought to also check change permission on the dataset of the gloss. Do we need full permission checks on all urls?

Probably the second checks code can be removed since it's not accessible outside of signbank? (It isn't always clear what other code might want to call the function.)

Probably we need some tests to check all the ajax urls to make sure they can't be evaluated without permission? There are lots of ajax urls that aren't included in the API.

[vanlummelhuizen]

@susanodd

When there is a permission_required decorator, you know the user is authenticated because by definition unauthenticated users do not have any Permissions set. The lines if not request.user.has_perm('dictionary.change_gloss'): are also not necessary if it is handling the same permission.

But the way they are used in the code you provided, there is no check for an object permission, only a global permission. There is a Guardian decorator (https://django-guardian.readthedocs.io/en/stable/api/guardian.decorators.html#permission-required) but I don't think it is able to do a permission check on the Dataset of a Gloss.

Therefore, I think you need to check object permission at the top of the function and remove any permission_required decorators. A login_required decorator may come in handy here.

[susanodd]

@vanlummelhuizen I removed all the "internal method" permissions since it's in the decorator already.

Need to work out the object permission now.

We've been checking that the user has change_permission for the dataset of the gloss (gloss.lemma.dataset).

But since the url has a gloss id in it, the object is not known yet.

The little mini updates are all ajax calls used inside the template. It's not possible to pass the actual objects. (Things are always converted by Django to be ids instead of objects if you want to use them in javascript.) Unless something else needs to be passed around?

HEY, is it possible to actually use lemma__dataset and gloss__id inside the decorator for permission?

[susanodd]

@vanlummelhuizen I think we (you) thought about this once upon a time, that there could be a group for each dataset. Then the test would just check that the user belonged to the group that can change the glosses in the dataset. There are only a handful of users that can edit glosses in NGT. (Not including staff/superusers that are indirect. We only do testing stuff on the development servers, not on the production server.) There used to be a group NGT. But somebody deleted it.

[susanodd]

@Woseseltops there is a discrepancy with your claim that the user will need to press the "save text fields" button say 100 times. The user can potentially update the text in 6 or more fields for each gloss, so that's say potentially 20-30 keystrokes per gloss. So the "save" is trivial in comparison to all the typing. It's ridiculous to imagine a user typing in 300 keystrokes and not saving anything, moreover, needing to keep scrolling up to the top to get to the single save button. Scrolling or a button "go to top" to make it faster to navigate to get to the save button is also silly. Why not use the gloss's own save button?

[susanodd]

@susanodd

When there is a permission_required decorator, you know the user is authenticated because by definition unauthenticated users do not have any Permissions set. The lines if not request.user.has_perm('dictionary.change_gloss'): are also not necessary if it is handling the same permission.

But the way they are used in the code you provided, there is no check for an object permission, only a global permission. There is a Guardian decorator (https://django-guardian.readthedocs.io/en/stable/api/guardian.decorators.html#permission-required) but I don't think it is able to do a permission check on the Dataset of a Gloss.

Therefore, I think you need to check object permission at the top of the function and remove any permission_required decorators. A login_required decorator may come in handy here.

Okay, I've modified all the relevant functions (those in update.py that implement the individual mini update urls, plus the specific functions that do the updates in update_glosses.py) I added the check that the user has change_dataset permission for the dataset the gloss is in. I left the decorator change_gloss. That essentially is the same as the user being in group Editor. (But then also need to be able to edit e.g., NGT)

I'm not sure about the "accept any permission". In the User Profile I modified that (when the tokens were introduced) to only show the explicit permissions. Otherwise, it's weird. I can see that it's needed for change_gloss. But for change_dataset, the manager of the dataset should have given change permission. (The superusers are the exception here. But originally we were supposed to be using various test users to test things. But we've all forgotten the passwords.)

[Woseseltops]

Would it be today to turn this pull request into a draft? I ask because I notice:

• You are still working on it
• We still need some input from Ulrika about functional stuff

[susanodd]

Would it be today to turn this pull request into a draft? I ask because I notice:

• You are still working on it
• We still need some input from Ulrika about functional stuff

I set up signbank-test and put this branch there.

I fixed the permissions as per review. Yes, waiting for input from Ulrika. I only want to add more phonology to it.

[susanodd]

I'm going to merge this with master on Monday.