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

Reduce duplicate queries on instrument list view #143

Merged

Conversation

dchiller
Copy link
Contributor

@dchiller dchiller commented Aug 15, 2024

This PR refactors the InstrumentList view and associated templates to reduce the number of database queries. See profile:
image

We now select and prefetch related objects, using a custom related objects manager to filter instrument names on the backend, rather than during template rendering (the cause of a great many of the duplicate queries).

The PR also adds some additional python development dependencies to the pyproject.toml file:

  • djlint was configured for html template formatting but had not been added as a dependency
  • stubs for the django and requests packages are added for type checking

Closes #141

…t view

- Uses existing total instrument count from paginator to get `instrument_num`
context variable
- Uses custom prefetch manager to filter instrument names by active language
on the back-end (simultaneously remove this check from the template rendering)
- preselect the related AVResource thumbnail object
- extract repeated code to determine "active language" into separate function

Also implements minor formatting and typing changes.

Refs: DDMAL#141
@dchiller dchiller force-pushed the i141-reduce-queries-instrumentlist-view branch from 9720710 to c34dcf5 Compare August 15, 2024 15:33
@dchiller dchiller changed the title I141 reduce queries instrumentlist view Reduce duplicate queries on instrument list view Aug 15, 2024
@dchiller dchiller marked this pull request as ready for review August 15, 2024 15:36
@kunfang98927
Copy link
Contributor

It looks good! @dchiller I only have a few questions:

  • When I run docker compose build, it has the following error message:
image

But when I change the poetry version to the latest version (pip install poetry==1.8.3) in the 4th line of install-packages.sh, docker compose build can then be run successfully.

Do you think we need to upgrade poetry? Is the error message related to the poetry version?

  • This question is not related to this PR. In Django DevTool, the static files tab always has 500: internal server error. Do you know what the reason is?
image

@dchiller
Copy link
Contributor Author

Do you think we need to upgrade poetry? Is the error message related to the poetry version?

Whoops! You're right, package-mode = false support was added in poetry 1.8, which is what I have locally, but not what the container build uses. I'll add this to the PR. Thanks!

@dchiller
Copy link
Contributor Author

dchiller commented Aug 15, 2024

  • This question is not related to this PR. In Django DevTool, the static files tab always has 500: internal server error. Do you know what the reason is?

I'm seeing the following error in the app container logs when I try to access the "Static files" panel:

django.core.exceptions.SuspiciousFileOperation: The joined path (/js/GoogleTranslate.js) is located outside of the base path component (/virtual-instrument-museum/vim-app/VIM/static)

I'll take a closer look tomorrow.

@dchiller dchiller merged commit 30e5a4f into DDMAL:develop Aug 16, 2024
2 checks passed
@dchiller dchiller deleted the i141-reduce-queries-instrumentlist-view branch August 16, 2024 11:50
@dchiller
Copy link
Contributor Author

@kunfang98927 The issue with the static files turns out to be here:

<script src="{% static "/js/GoogleTranslate.js" %}"></script>
<script type="text/javascript"
src="//translate.google.com/translate_a/element.js?cb=googleTranslateElementInit"></script>
<script src="{% static "/js/LanguageTooltip.js" %}"></script>

The parameter to the static tag should be a relative and not an absolute path (ie. if we get rid of the opening slash on "/js/GoogleTranslate.js" and "/js/LanguageTooltip.js", it's fine).

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

Successfully merging this pull request may close these issues.

Reduce queries on Instrument List view
2 participants