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

Allow users to edit documents' fulltext fields (used in search) #139

Merged
merged 4 commits into from
Nov 7, 2024

Conversation

Restioson
Copy link
Collaborator

This allows users that have the suitable permission to edit the fulltext field of Documents. This field is used when searching, so it allows users to remove frontmatter from PDFs which are irrelevant, or paste a plaintext version for a Google Doc link, for example. The field is only shown to those with the relevant permission to avoid confusing people who may not understand what it is for. When a PDF is edited for a Document, unless the fulltext is also edited at the same time (within the same form submission), it will take priority and overwrite the old fulltext (even if edited prior).

This PR should be rebased on top of #138 when that PR is merged, as #138 is essentially a cherry-picking of some dev experience fixes I made while working on this PR.

Fixes #137.

@Restioson Restioson self-assigned this Nov 5, 2024
@Restioson Restioson marked this pull request as ready for review November 5, 2024 13:00
Copy link
Contributor

@friedelwolff friedelwolff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting hair about commit wording: The term "fixture" you use in 56a5014 will seem to refer to the Django feature with that name: https://docs.djangoproject.com/en/5.1/topics/db/fixtures/ I guess you use it here in a more general sense.

Maybe something like "Reuse test institution" will be fine or anything in similar vein.

app/general/tests/test_document_admin.py Outdated Show resolved Hide resolved
app/general/models.py Show resolved Hide resolved
app/general/admin.py Outdated Show resolved Hide resolved
app/general/admin.py Outdated Show resolved Hide resolved
@Restioson
Copy link
Collaborator Author

Splitting hair about commit wording: The term "fixture" you use in 56a5014 will seem to refer to the Django feature with that name: https://docs.djangoproject.com/en/5.1/topics/db/fixtures/ I guess you use it here in a more general sense.

Regarding this, I say fixture as it is a common way to refer to setup and teardown in testing frameworks. E.g., from the unittest docs:
image

Happy to change it to something else if you feel there's a clearer term. I think it's worth mentioning that Django fixtures are not completely orthogonal to the unittest ones though!

@friedelwolff
Copy link
Contributor

Don't worry too much about the term "fixtures", but maybe keep in mind in future, that in the Django world, I think most people will think of the output of the management command that dumps data, rather than a general test fixture.

@Restioson
Copy link
Collaborator Author

I added a very small change to the Makefile here (useful for testing) in ffb22ed - happy to cherry-pick off to another branch as well

app/general/models.py Outdated Show resolved Hide resolved
…nt form

This is by far the slowest part of validation, and happens even if
there are other issues with the form. Because Django disables
client-side validation for editing model forms (see comment), we
need server-side validation to be fast, so we delay PDF text
extraction until all errors are fixed.
This prevents the need for having to call `Institution.objects.create` with a
_globally unique_ name in each test (previously, reusing a name would cause an
error)
This allows users that have the suitable permission to edit the fulltext field
of Documents. This field is used when searching, so it allows users to remove
frontmatter from PDFs which are irrelevant, or paste a plaintext version for a
Google Doc link, for example. The field is only shown to those with the relevant
permission to avoid confusing people who may not understand what it is for.
When a PDF is edited for a Document, unless the fulltext is also edited at the
same time (within the same form submission), it will take priority and
overwrite the old fulltext (even if edited prior).
The migration and appto migrate to can now be specified by setting the
`migration_args` Makefile variable on the command line, e.g. `make migrate
migration_args="general 0013"`.
@Restioson Restioson merged commit 4ec3284 into main Nov 7, 2024
7 checks passed
@Restioson Restioson deleted the feat/edit-fulltext-field branch November 7, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow editing of full-text search field
2 participants