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

adding introduction to citing author and citing work #478

Draft
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

acholyn
Copy link
Collaborator

@acholyn acholyn commented Sep 26, 2024

closes #383

@acholyn acholyn requested a review from tcouch September 26, 2024 13:04
Copy link
Collaborator

@tcouch tcouch left a comment

Choose a reason for hiding this comment

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

When adding or updating an introduction on a citing_work or citing_author, if it includes a mention of a bibliography item, you get the following error:

File "/app/rard/utils/basemodel.py", line 319, in link_bibliography_mentions
     for ant in antiquarians:
UnboundLocalError: local variable 'antiquarians' referenced before assignment

We can't really assign an antiquarian to a citing author or citing work, but we would like to link bibliography items in the same way. We'll have to expand the link_bibliography_mentions method to handle this.

src/rard/utils/basemodel.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you look at the Antiquarian detail page, there's a htmx trigger on the bibliography item list which refreshes it when the introduction is updated. It just means you get to see the updated bibliography without having to refresh the page.

Comment on lines 177 to 178
if self.ant_pk:
context["antiquarian"] = Antiquarian.objects.get(id=self.ant_pk)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Repeated lines here. Was this supposed to do something different?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

May be a holdover from when I was going to try something and then either it needs to be updated or is no longer relevant

@@ -163,6 +163,7 @@ class BibliographySectionView(LoginRequiredMixin, PermissionRequiredMixin, ListV
context_object_name = "bibliography_items"
permission_required = ("research.view_bibliographyitem",)

# todo: update this to dynamically work with authors and ants
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs doing doesn't it? You could use this approach to pass kwargs depending on the URL used to call the view:
https://docs.djangoproject.com/en/5.1/topics/http/urls/#views-extra-options

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently this view uses research/partials/antiquarian_bibliography_list.html which includes a htmx process for refreshing the content that points to the antiquarian:bibliography url. You'll need to either switch to a different template depending on the related model in the view, or pass something in the context data that can be used to switch the url in the template.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've bypassed the need for the antiquarian_bibliography_list template with citing authors by including the equivalent in the detail template. My thinking was that htmx doesn't need a whole new template to update the content.
I think I've gotten confused somewhere along the way and am trying to blend two different strategies to this solution

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is that updating the intro triggers the bibliography to refresh via this view, and the html content that gets swapped in has two problems:

  1. No bibliography items are returned because the view can't handle citing authors at the moment
  2. The template has hx-get="{% url 'antiquarian:bibliography' pk=antiquarian.pk %}" in it, but we need that to point to the citing_author:bibliography url if it's being rendered for the citing author detail page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To replicate the issue I'm talking about:

  1. Edit a citing author's introduction including a mention of a bibliography item
  2. Save the changes
  3. You should see the bibliography section at the bottom automatically update, but without any content
  4. Note the url in the #bib-list element at the bottom of the page

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, changes hadn't been pushed! Think these have been addressed in the latest one; I've used a get_templates function so it points to the right one and put the citing author section in a new template

Comment on lines 662 to 666
path(
"<pk>/bibliography/",
views.BibliographySectionView.as_view(),
name="bibliography",
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

If using the passing extra options approach, I think this could work:

Suggested change
path(
"<pk>/bibliography/",
views.BibliographySectionView.as_view(),
name="bibliography",
),
path(
"<pk>/bibliography/",
views.BibliographySectionView.as_view(),
{"related_model":"citing_author"},
name="bibliography",
),

@acholyn acholyn marked this pull request as draft December 18, 2024 07:18
@acholyn
Copy link
Collaborator Author

acholyn commented Dec 18, 2024

I've also just noticed that we don't have a shortcode for citing authors in mentions, so how do we mention them? Or is this just another thing to do for this PR

@tcouch
Copy link
Collaborator

tcouch commented Dec 18, 2024

I've also just noticed that we don't have a shortcode for citing authors in mentions, so how do we mention them? Or is this just another thing to do for this PR

I guess the researchers have just never needed to mention citing authors. Is there a reason we would want to add it?

@acholyn
Copy link
Collaborator Author

acholyn commented Dec 18, 2024

Is there a reason we would want to add it?

Perhaps not, I suppose the researchers can always request this in future

@acholyn
Copy link
Collaborator Author

acholyn commented Dec 18, 2024

Possible qol: when clicking the refresh, it should do a loader since typically on a CA it will already be updated and won't actually change

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.

Adding text boxes to citing authors and citing author works
2 participants