Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1270: Signbank empty database. #1316

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

#1270: Signbank empty database. #1316

wants to merge 14 commits into from

Conversation

susanodd
Copy link
Collaborator

@susanodd susanodd commented Aug 22, 2024

Modified permissions. Command to empty tables, command to remove users except for developers.

[ORIGINAL, SKIP THIS]
The empty database is created with the same name as the test database, it does not overwrite the original database.

Then to use the new empty database instead, you need to copy it to overwrite the real signbank.db (on signbank-test this was done.) It is intended that other developers (the VGT-ers will start out with the empty database).

Then a new test database need to be made (the create_test_db throws away more table contents.)

This is a TEST DATABASE

This is on signbank-test with an EMPTY database. (Well, nearly empty, the interface translations are still there..)

ALL TESTS SUCCEED!!

I was able to create a gloss in tstMH.

NEXT

Remove interface languages. This will require a migration, so this branch won't be compatible with other branches.
The settings for this server will need to change the model translation settings.
I'll see if this can be a script so it can be used optionally.

Some of the permissions probably need to revert to what they were. It was a problem at first because there was no data. The Dataset pages did not show any of the datasets and the original permissions did not work.

Specific to VGT, see if they want to retain the VGT dataset glosses.
This would facilitate sharing in the future since the gloss IDs would be retained.
One of the issues of @ocrasborn was to add extra relations to other sites (#93)

[READ THIS, UPDATE PER 02-09-2024]
After the new permissions (#1281, #1282), the empty database has zero permissions to choose from in the Admin.
All the "change_dataset, view_dataset, etc have disappeared from the permissions. It's not possible to grant users permissions in the admin because there are no permissions.
Does the "vacuum" command do this? Or are too many tables erased, but not shown in the log of tables erased?

Modified permissions. Command to empty tables, command to remove users except for developers.
Put permissions on database back to what they were.
@susanodd
Copy link
Collaborator Author

susanodd commented Aug 22, 2024

@Woseseltops this is on signbank-test.

If you look at the admin you can see what the contents is.

I need to make a migration to remove the extra interface languages.
(Go back to just EN and possibly NL. Or what to do here?)
The end developer should be able to decide what languages to include.
English must be there because the templates are in English and that is the language from which the other languages are translated in the PO files.

All of the Field Choices and Handshapes are in the database. It won't work without them. Or at least some. (They can't be empty.)
(At the very least they need machine values 0 and 1 as constants.)

I suspect Django will make a migration as soon as we update the model translation languages settings.
(But then this branch will become incompatible with the master branch.)
Perhaps another branch similar to ASL where it's an empty signbank with only English plus instructions for adding interface languages.

We probably need some basic data in tstMH. Just because it's not possible to test everything otherwise.
I created one gloss.

@susanodd
Copy link
Collaborator Author

susanodd commented Aug 23, 2024

@vanlummelhuizen can we make a conditional migration to remove the model translations that are not needed?
Then the users that start with an empty database can keep up to date with the master branch.
Of course, they could add their own model translation languages, so then they'd have extra migrations on their installation.
The other installations will need to maintain the "po" files in case they add back a language. If we'd get French translations that would be cool, too.

(I write this in general although this is for the VGT site. #1270 )
Can we have Django set the migration to "applied" if the model translations don't match (either direction)?

@susanodd
Copy link
Collaborator Author

susanodd commented Aug 23, 2024

@vanlummelhuizen I needed to alter the permissions in the Dataset Select page when I started using the empty signbank. It did not want to display anything.

Then I went ahead and changed them elsewhere to be the same. But then the tests didn't work, so I changed the ones in the tests (manage dataset tests). (Although I had to remove the checks on "messages" returned because it wasn't showing any messages. So it merely checks the status of the operation now.)
(But then I needed to revert permissions back to what they were elsewhere. It was the "assign_perm" command that seems to have changed. So possibly in all our migrations something is messed up, as you are already aware in the other issue you are working on with the permissions.)

Perhaps you can check out the permissions? [SEE CODE, ALL TESTS WORK NOW]

I removed users in a command, and I left more tables in the database (create_empty_db), based on create_test_db but it didn't work with the users removed, since I couldn't login anymore, so I left the development team, plus the future user of the VGT site.) I removed datasets by hand. It didn't work to remove users by hand. (Some error about a content type on some users, internal to Guardian.) The removal of the datasets will be needed to be added to the create_empty_db command. Maybe we just leave the test dataset? Plus there likely need to be some "start" data in the database because it's not possible to check that everything works without any glosses. It would be good if we had some "representative data" in the test dataset. I made the one gloss I created in tstMH be public.

@susanodd
Copy link
Collaborator Author

I left the Handshape, Field Choice, Semantic Field, and Derivation History choice tables in the database.
Although other sites may want to change these or add translations, leaving these objects would facilitate sharing data between signbanks, at least for the phonology. Although new gloss ids would be needed. Or some configuration of gloss ids.

@susanodd
Copy link
Collaborator Author

susanodd commented Aug 23, 2024

[CODE DETAILS RELATED TO PERMISSIONS, THIS SHOWS DETAILS, SKIP THIS]

In the context_processors.py file:

for dataset in Dataset.objects.all():
    permissions_for_dataset = get_user_perms(request.user, dataset)
    print(permissions_for_dataset)

results in this kind of permission:

<QuerySet ['add_permission', 'change_permission', 'delete_permission', 'view_permission']>

NOT the permissions that look like "change_dataset".

So there are different looking permissions floating around.

@susanodd
Copy link
Collaborator Author

susanodd commented Aug 23, 2024

[SKIP THIS COMMENT, SCROLL DOWN TO NEXT, READ CODE TO SEE FIXES NEEDED]
[RESOLVED: PERMISSIONS FIXED TO USE BOTH KINDS WHERE NEEDED TO WORK]
One of the tests that FAILS is the test that creates a gloss. The CRUD test.

I have no idea how to get it to work.
It's not creating a gloss. [FIXED BY CHANGING PERMISSIONS SEE CODE CHANGES IN PULL]

However, it is possible to create a gloss in the interface.

I don't know if the error that the test does not work is related to permissions or not.

URGH!!!!!!

[DESCRIPTION OF BUG AND FIX]
So to UPDATE a gloss, this is needed:

'change_dataset' in get_user_perms(request.user, dataset)

So a totally different permission than in other parts of the code.
What is this?
How can this be so inconsistent?

Now I have no idea how to consistently code the permissions tests.

I'll make it so the code respects the tests, since the test actually catch broken code.
But this is worrying that it all seems inconsistent.

So above, "change_permission" on the dataset was needed, but here, "change_dataset" permission was needed.
To debug this, I used print statements to find out what was actually in the permissions set of the get_user_perms.

@susanodd
Copy link
Collaborator Author

@Woseseltops

ALL OF THE TESTS ARE SUCCESSFUL !!!

@susanodd
Copy link
Collaborator Author

@Woseseltops if you browse the changes to files, most are permissions.

I have no idea WHY two different kinds of permissions are needed.
They also don't match what is done in #1282

@vanlummelhuizen
Copy link
Collaborator

@susanodd

Many of the changes here are also in #1282, although not completely identical. They differ on this: you have view_permission where I have view_dataset.

When I dive directly into the database (bin/develop.py dbshell) I can exactly see what the difference is. First find all permissions with text permission:

sqlite> select * from auth_permission where codename like '%permission%';
id|content_type_id|codename|name
1|1|add_permission|Can add permission
2|1|change_permission|Can change permission
3|1|delete_permission|Can delete permission
142|44|add_userobjectpermission|Can add user object permission
143|44|change_userobjectpermission|Can change user object permission
144|44|delete_userobjectpermission|Can delete user object permission
145|45|add_groupobjectpermission|Can add group object permission
146|45|change_groupobjectpermission|Can change group object permission
147|45|delete_groupobjectpermission|Can delete group object permission
221|1|view_permission|Can view permission
282|45|view_groupobjectpermission|Can view group object permission
283|44|view_userobjectpermission|Can view user object permission

Then see what ContentType these refer to:

sqlite> select * from django_content_type where id in (1,44,45);
id|app_label|model
1|auth|permission
44|guardian|userobjectpermission
45|guardian|groupobjectpermission

As you can see none of the permissions with permission in the codename refers to Dataset.

Conclusion: The codename view_permission refers to the Permission that lets users view objects of type Permission, not the Permission that lets users view objects of type Dataset.

So, I think all view_permissions in your code should be view_dataset.

@susanodd
Copy link
Collaborator Author

@vanlummelhuizen Yes, it looks like that.

But some of the code does not run at all unless the kind with _permission is used.

On the initial startup, it would not show any datasets at all in the Select Dataset page. I had to change it to the _permission one in order for it to show anything.

@vanlummelhuizen
Copy link
Collaborator

@susanodd I can only imagine that this is coincidental, probably because the current user is a staff member or superuser. The problem that is does not show anything lies elsewhere.

Permissions with _permission in the codename are not a kind of permission but rather a normal permission that says what users may or may not do with objects of the Permission model, and those users are usually staff or superuser.

@susanodd
Copy link
Collaborator Author

@Woseseltops can you check that there isn't anything signbank-specific to Radboud in the empty database?

I found one instance of "site" that is the url of signbank. (That is inside the database, not a setting.)

If you use the new Amazon-tool, can it be with the empty database to check it out?

@susanodd
Copy link
Collaborator Author

susanodd commented Aug 28, 2024

@susanodd I can only imagine that this is coincidental, probably because the current user is a staff member or superuser. The problem that is does not show anything lies elsewhere.

Permissions with _permission in the codename are not a kind of permission but rather a normal permission that says what users may or may not do with objects of the Permission model, and those users are usually staff or superuser.

I still don't know why the select dataset page was not working.
Did some "data" or "specific permissions like owner" get thrown out when users were deleted?

I need to figure out if too many things were deleted to make it empty, causing it to not work.
There are some commands that fall back to the default dataset. But NGT is empty now.
Maybe like that other example (the signlanguage query issue) maybe if the dataset is empty some things don't work?
Otherwise, there needs to be at least one sign. NGT was the only public dataset.

@susanodd
Copy link
Collaborator Author

susanodd commented Aug 28, 2024

@susanodd I can only imagine that this is coincidental, probably because the current user is a staff member or superuser. The problem that is does not show anything lies elsewhere.

Permissions with _permission in the codename are not a kind of permission but rather a normal permission that says what users may or may not do with objects of the Permission model, and those users are usually staff or superuser.

What about the Manage Dataset page?
Those needed to be changed also in the tests.

In the empty database, I noticed that it had made me owner of something. I had removed all the owner users of NGT, which is empty but not deleted.

@susanodd
Copy link
Collaborator Author

@susanodd I can only imagine that this is coincidental, probably because the current user is a staff member or superuser. The problem that is does not show anything lies elsewhere.

Permissions with _permission in the codename are not a kind of permission but rather a normal permission that says what users may or may not do with objects of the Permission model, and those users are usually staff or superuser.

Okay, so I switched to this branch with a "normal" database.
Guess what? The select dataset page shows no datasets.

I'll set the permissions back to what they were.
Hopefully it is something coincidental. Or related to the datasets being empty.

@susanodd
Copy link
Collaborator Author

susanodd commented Aug 28, 2024

I set the permissions back, although they now match the other issue about the permissions.
(By accident since I was inspecting code to see what permissions there were available to know what could be checked for.)

As before all the tests using the empty database pass.
And this branch also works for the real database.

@susanodd
Copy link
Collaborator Author

susanodd commented Aug 30, 2024

@Woseseltops since you have experience with a totally different signbank branch, ASL, perhaps you can decide what to do for VGT.

Once they start modifying the settings, it ought to remain possible to pull from the master branch.
They will probably also want to keep the po file up to date. Probably a 'be' po file.
But then that will need to be filled with thousands of translations because they start out empty.
Or they could copy the 'nl' file and edit that to start.
But they will need to keep doing this in order to keep their local signbank up to date.

Also the migrations for the model translations will need to be conditional, and also on master.
We could create a migration to add 'be' but have it be conditional and not add it to global signbank.
Then it would also be on master.
(Likewise as already pointed out, to remove the extra translation languages.)

In the empty database (signbank-test) the About pages have content in the Pages model.
After the extra languages are migrated out, the About content should be made empty, or a helpful "fill contents here".
The pages can't be deleted because they are needed for the urls to work. (That is what is wrong with the database of #1270 there are zero pages because the Page table was deleted or removed.)

@susanodd
Copy link
Collaborator Author

susanodd commented Sep 10, 2024

I added more tables to the KEEP list as discovered by @vanlummelhuizen. (For some reason this did not affect the tests on the normal database, but they need to be explicitly in the KEEP list when creating an empty database.)

Then performed the steps above. (#1316 (comment))

All of the tests work now.
Only the dataset tstMH remains and only the developer users remain, plus Divya.
(It's unclear if the VGT team would like users to already be in the database for themselves? Then we can remove ourselves as a final step.)

The empty database is on signbank-test.
writable/database/signbank.db

The name of the dataset can be changed to something generic. I changed the server specific to have dataset 1 tstMH be the default.
The About pages are still the same. (What to do about the copyright stuff? Or mention of signbank on github, publications, etc.)
There may be hidden email references in the code.)

And of course migrating Chinese, Arabic, and Hebrew out. (#1320)

@Woseseltops communicate however you did this before with the empty database.

I like the method of @vanlummelhuizen much better of course. But I'm not able to develop that myself, I think it should be tested on the Amazon temporary servers.

@susanodd susanodd added completed and removed DRAFT labels Sep 10, 2024
@vanlummelhuizen
Copy link
Collaborator

Or onto your tensusers account so that we can copy it elsewhere?

It is here: /vol/tensusers/mhulsbosch/signbank/signbank.db

I browsed the json files, I suspect that the pages data is going to need the actual "contents" fields for the different languages, albeit empty/single space character.

I don't think so. The handling of pages not in a urls.py somewhere is done by PageFallbackMiddleware in https://github.com/Signbank/Global-signbank/blob/master/signbank/pages/middleware.py: when the normal flow results in a 404 it tries to find the page in Pages.

Is it wise to edit the early migrations? The migrations can no longer be applied again retroactively.
(Will this cause any problems with the master database if the migrations that it has applied in the past have been modified?)

This is a good question. I assumed editing early migrations is possible. These migrations are already in the Global-Signbank instance so they will not be applied again there. Also, I tried to make sure the end result is not different from the database we already have in terms of structure/schema. If you find anything in my edits that makes you think otherwise, please let me know.

The models Handshape, SemanticField, DerivationHistory are all objects and no longer field choices. They will need json files too. And the field choices for them removed. (Otherwise there will be lots of type errors because they need to be objects of these models.)

Good point. I will look into that.

@vanlummelhuizen
Copy link
Collaborator

I added fixtures for Handshape, SemanticField and DerivationHistory. Again, it is on branch make_installable.

This one ought to be done using the new Amazon temporary server pull request setup of @Woseseltops.

Would be cool to see a fresh install on AWS this way. I first need to find out how that works.

@susanodd
Copy link
Collaborator Author

susanodd commented Sep 12, 2024

@vanlummelhuizen I just put your signbank.db (empty as on tensusers) onto signbank-dev.

I revised the settings in the server_specific.py file (for the dev server only) as shown above for the dataset settings.

I'm not sure how I can login now?
All the pages in the menus show up properly!
No datasets in the header, since none are public.

It's on signbank-dev. Check it out!

@vanlummelhuizen
Copy link
Collaborator

I'm not sure how I can login now?

You need to create a super user first: https://docs.djangoproject.com/en/5.0/intro/tutorial02/#creating-an-admin-user

@susanodd
Copy link
Collaborator Author

susanodd commented Sep 12, 2024

Okay, quite a lot of tests fail. (!!)

Many are that the Dataset does not exist.
(But I changed the settings as shown above, so they should match the database, right?
Or perhaps some of the code isn't using the server-specific settings.
I didn't change them in default because then it won't be able to pull from master anymore.)

In the pull-down menus, lots of pages show up in the pull-downs in spite of an anonymous user not being able to see them. (The extra group permissions you set in the admin for the pages, hides them from the menus if the user is not logged in.)

@susanodd
Copy link
Collaborator Author

susanodd commented Sep 12, 2024

@vanlummelhuizen could you have a look at the settings files?
(Slightly related to this, I'm adding the basic structure for the animation issue. I've encountered some imports of settings and I'm not sure whether they are correct. For example,

from signbank.settings.base import WRITABLE_FOLDER, GLOSS_VIDEO_DIRECTORY, GLOSS_IMAGE_DIRECTORY, FFMPEG_PROGRAM

Those things aren't defined in base.
They're in default.
Except how does Django determine whether to retrieve the settings from the specific server_specific.py file?
(Those override the defaults.)
(I am wondering about this because it seems it has something to do with some of the tests not working on the empty database.)

Here is a setting that is not related to anything:

DEFINITION_FIELDS = ['general', 'noun', 'verb', 'interact', 'deictic', 'modifier', 'question', 'augment', 'note']

@Woseseltops
Copy link
Collaborator

Summary of a conversation with @susanodd :

  • This functionality is mainly for new Signbank installations by new teams, so the approach where you don't need an existing database will be more useful in most cases
  • Various tests (and also functionality?) require at least one dataset, so it would convenient if this tool/command created that

Optional functionality: also have this tool/command create a page with copyright info

@vanlummelhuizen
Copy link
Collaborator

  • This functionality is mainly for new Signbank installations by new teams, so the approach where you don't need an existing database will be more useful in most cases

Do you mean what is done in make_installable?

  • Various tests (and also functionality?) require at least one dataset, so it would convenient if this tool/command created that

When creating a database from scratch as in make_installable, a default dataset is created: "Your dataset", YDS. See https://github.com/Signbank/Global-signbank/blob/master/signbank/dictionary/migrations/0027_auto_20180803_1340.py

But it would be best if the tests are creating a test-dataset themselves, independent of the contents of the (test-)database.

@vanlummelhuizen
Copy link
Collaborator

Okay, quite a lot of tests fail. (!!)

I did some testing and one problem was a typo in the DEFAULT_DATASET setting of the server_specific.py file I gave earlier. It should be:

DEFAULT_DATASET = 'Your dataset'

(lower 'd' of dataset)

@susanodd
Copy link
Collaborator Author

susanodd commented Sep 17, 2024

Yes, some of the older tests use NGT. (Kind of dangerous.) Some do create a new dataset. I agree that this needs to be for all. I an work on that.
Good catch with the capital d typo!

@vanlummelhuizen you're creating a default language and a sign language?
Those are needed in order to create a dataset.
Do the tests need to take into account having multiple languages or just one languages?
At the moment, it's blurry because the translation languages are retrieved from the default dataset.

@vanlummelhuizen
Copy link
Collaborator

To solve quite some tests I add language_code_2char values to migration 0053, see 103da9c

@susanodd
Copy link
Collaborator Author

To solve quite some tests I add language_code_2char values to migration 0053, see 103da9c

That will also solve many problems in the templates, since all the little annotation dictionaries map the 2-char to a string.

@vanlummelhuizen
Copy link
Collaborator

There is one test that still fails:

def test_Import_csv_update_gloss_for_lemma(self):

I think this is because the test expects something that will not happen: the test sends a form (stage 2) with changes that were pretended to be extracted from a CSV in the stage before (stage 1). However, the thing that is tested (

self.assertContains(response, 'No changes were found.')
) will never happen because in views.py the text 'No changes were found.' is only used in stage 1, not stage 2. See
elif stage == 1 and not changes and not error:
# no changes were found in the input file. print a message as feedback
# this is needed in order to have output that can be tested for in the unit tests
messages.add_message(request, messages.INFO, _('No changes were found.'))
and

@susanodd Do you agree and if yes, do you have an idea to solve this?

@susanodd
Copy link
Collaborator Author

susanodd commented Sep 18, 2024

@vanlummelhuizen @Woseseltops I copied the empty database I had created onto my tensusers account.

/vol/tensusers2/seven/signbank_empty_180924.db

I needed to put the animation code on signbank-test because the CORS isn't set up properly on signbank-susan and there is no ajax lookahead. (Some bug it thinks that it is on a different site.)

@susanodd
Copy link
Collaborator Author

@vanlummelhuizen that's weird about the test. I will need to check it out. For some reason it works on the normal databases.
Perhaps I modified something in the code that didn't trickle down.
I haven't modified that code in quite a while. But some similar constraints checks for lemmas got implemented for the API, so it could be that I modified ("improved") some code but didn't check it.
I also added some prototype spreadsheet rows to the template, so I could have messed up something.

@susanodd
Copy link
Collaborator Author

susanodd commented Sep 19, 2024

The test is testing whether there is one translation language. So probably something else is going on.
I don't think it gets as far as stage 2.

response = client.post(reverse_lazy('import_csv_update'), form_data, follow=True)
if count_dataset_translation_languages > 1:
print('More than one translation language, attempt to update a lemma translation')
self.assertContains(response, 'Attempt to update Lemma translations')
else:
print('Only one translation language, no changes found')
self.assertContains(response, 'No changes were found.')

if count_dataset_translation_languages > 1:
print('More than one translation language, attempt to update a lemma translation')
self.assertContains(response, 'Attempt to update Lemma translations')
else:
print('Only one translation language, no changes found')
self.assertContains(response, 'No changes were found.')

Yes, something needs to be modified for the tests because it assumes the dataset is NGT and has two languages. (That's not generic enough.)

[BLAH BLAH]
Sometimes is is necessary in the tests to have Follow being True or else False in order for it to "complete" whatever it is doing in the template, or not. I think this is also to do with when the messages are displayed "inside the test" since it's not the same as a real template.

It could even be that the template area for messages has become hidden or behind other stuff. I could have modified something for dark mode, or else for showing the scroll bar and not noticed. (Those are things that can hide the messages, also the "affix". I have no idea if that is relevant here, since it's a virtual template. That's why the tests just look to see if the string is contained, since they don't parse the template.)

The tests also assume the messages are in English. So if your own account that is running the test has a different language set up, that will affect the language of the messages.

That the template has the text "No changes were found" is different than that the test also has this.
The test is testing the "message" that shows this.
Some of the messages were in fact added to the code in order to be able to have the tests test something.
So here, it's the message that is being tested. The stage in the template is being ignored.
(Written without testing.)

Some of the CSV routines have a bail-out. So in the gloss update, if a column is not processed by the code, say the user tries to update the senses (this is not available, only creation is available), then the code skips those columns.
For the lemmas, both translation languages are required (For NGT). So that it says No changes were found is not accurate.
(At the time, it was too much error checking code, excessive feedback, so it just says No changes if it can't process them. Probably "Nothing to do." would be more appropriate.)

The routines for the API that do constraint checks or need to check existence of field choices, also report back about this when the operation cannot be performed. This is also translated. We don't have any tests for the API. But the constraint tests are needed.

@vanlummelhuizen
Copy link
Collaborator

vanlummelhuizen commented Sep 19, 2024

I made changes so that creating translation fields is done dynamically: 18a97e2 so that it also works with the following language settings

LANGUAGES = (
  ('en', gettext('English')),
  ('nl', gettext('Dutch')),
  ('zh-hans', gettext('Chinese')),
  ('he', gettext('Hebrew')),
  ('ar', gettext('Arabic')),
)

LANGUAGES_LANGUAGE_CODE_3CHAR = (
    ('en', 'eng'),
    ('nl', 'nld'),
    ('zh-hans', 'zho'),
    ('he', 'heb'),
    ('ar', 'ara'),
)

INTERFACE_LANGUAGE_SHORT_NAMES = ['EN','NL','官话','עִברִית','عربى']
MODELTRANSLATION_LANGUAGES = ['en','nl','zh-hans',
                              'he', 'ar'
                                ]

All tests pass except signbank.dictionary.tests.ImportExportTests.test_Import_csv_update_gloss_for_lemma.

@susanodd
Copy link
Collaborator Author

susanodd commented Sep 19, 2024

On a normal test database, even including the migrations for animations, this is what that test prints:

More than one translation language, attempt to update a lemma translation

That is the first branch of the code. Not the branch that looks for No changes found.
(So something is different in the empty database.)

Can you put the newest empty database on your tensusers again?

Is this set?

language_name = getattr(language, settings.DEFAULT_LANGUAGE_HEADER_COLUMN['English'])

I tried monkeying with the various settings to try to force it to get an error.

If you set the following to a constant "1" instead, then it gives the above error as you are getting

count_dataset_translation_languages = self.test_dataset.translation_languages.all().count()

So somehow this must be wrong.

@susanodd
Copy link
Collaborator Author

@vanlummelhuizen Okay, remove the "else" clause of that test, but keep the "if" clause.


        response = client.post(reverse_lazy('import_csv_update'), form_data, follow=True)
        if count_dataset_translation_languages > 1:
            print('More than one translation language, attempt to update a lemma translation')
            self.assertContains(response, 'Attempt to update Lemma translations')

Remove the "else" where it looks for "No changes found."
But that first test should be being tested. That's what happens on the non-empty database.

You're right, even if there is only one translation language (I tried ASL), it won't work for that. I suspect that is a remnant of old code that did additional tests on the input file. Once upon a time, it kept checking the dataset column again and again because it was allowed to modify multiple datasets.

@vanlummelhuizen
Copy link
Collaborator

Can you put the newest empty database on your tensusers again?

Done.

Is this set?

language_name = getattr(language, settings.DEFAULT_LANGUAGE_HEADER_COLUMN['English'])

Yes, it is taken from signbank/settings/server_specific/default.py.


Remove the "else" where it looks for "No changes found."

I did the following:

diff --git a/signbank/dictionary/tests.py b/signbank/dictionary/tests.py
index e84517d7..64de0252 100644
--- a/signbank/dictionary/tests.py
+++ b/signbank/dictionary/tests.py
@@ -699,9 +699,6 @@ class ImportExportTests(TestCase):
         if count_dataset_translation_languages > 1:
             print('More than one translation language, attempt to update a lemma translation')
             self.assertContains(response, 'Attempt to update Lemma translations')
-        else:
-            print('Only one translation language, no changes found')
-            self.assertContains(response, 'No changes were found.')
 
 
         # Prepare form data for linking to SEVERAL EXISTING LemmaIdgloss + LemmaIdglossTranslations
@@ -723,9 +720,6 @@ class ImportExportTests(TestCase):
         if count_dataset_translation_languages > 1:
             print('More than one translation language, attempt to update a lemma translation')
             self.assertContains(response, 'Attempt to update Lemma translations')
-        else:
-            print('Only one translation language, no changes found')
-            self.assertContains(response, 'No changes were found.')
 
 
     def test_Import_csv_new_gloss_for_lemma(self):

and now there is no failure. But then again, I just removed the failing test.

@susanodd
Copy link
Collaborator Author

@vanlummelhuizen it fails because it thinks the number of translation languages is not 2.
The normal database does the branch that is still there. You can see in the log for a normal database that it does that branch.
Does the test dataset have two translation languages? They are added in the code with "add". Does the make install do it via the fixtures?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants