-
Notifications
You must be signed in to change notification settings - Fork 3
Feature/fixed filter order #1704
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
Conversation
I know I just approved this, but then I remembered something I wanted to comment. I suspect your solution will not work when the field order in the definition is updated. Can you verify that this unit test passes? import shutil
import os
from addcorpus.python_corpora.save_corpus import load_and_save_all_corpora
def test_field_order_python_corpus(small_mock_corpus, admin_client, tmpdir, settings):
# check field order matches corpus definition
response = admin_client.get('/api/corpus')
corpus_data = next(c for c in response.data if c['name'] == small_mock_corpus)
field_names = [field['name'] for field in corpus_data['fields']]
assert field_names == ['date', 'title', 'content', 'genre']
# copy corpus definition into tmpdir
current_dir = os.path.join(settings.BASE_DIR, 'corpora_test', 'small')
shutil.copytree(current_dir, tmpdir)
definition_path = os.path.join(tmpdir, 'small_mock_corpus.py')
with open(definition_path, 'r') as definition_file:
definition_str = definition_file.read()
# replace `fields = [...]` line in file to change field order
definition_str.replace(
'fields = [date, title_field, content, genre]'
'fields = [title_field, content, genre, date]'
)
# save edited definition
with open(definition_path, 'w') as definition_file:
definition_file.write(definition_str)
# check order has changed
settings.CORPORA[basic_mock_corpus] = definition_path
load_and_save_all_corpora()
response = admin_client.get('/api/corpus')
corpus_data = next(c for c in response.data if c['name'] == small_mock_corpus)
field_names = [field['name'] for field in corpus_data['fields']]
assert field_names == ['title', 'content', 'genre', 'date'] Disclaimer: I haven't run it myself, so the test may need to be corrected in some way. |
looks good if you can fix the failing test! Taht should be no problem, feel free to merge after that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really mind merge migrations myself, but see #1139 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okayy I fixed the history but I got some strange notifications about my SQL database already containing the column position so I quickly hacked it a little bit with Xander's help, but it would be great if you could try and see if you can also migrate without issues to 0025?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds like you did not roll back the migrations before changing them. (I do that all the time, so this sounds familiar.)
The migration runs without issue on my machine, and migration code looks fine 👍
@Meesch is this ready to merge? |
Changes the behaviour of the Corpus serializer to maintain the order of the fields as they appear in the corpus definition. the read_only setting of the FieldSerializer was causing the order to disappear. This branch also contains the commits from
feature/deduplicate-corpora
, which was used to split off from (oops).