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

Fix/fragment ordering #375

Merged
merged 103 commits into from
Apr 25, 2023
Merged

Fix/fragment ordering #375

merged 103 commits into from
Apr 25, 2023

Conversation

acholyn
Copy link
Collaborator

@acholyn acholyn commented Jan 30, 2023

No description provided.

@acholyn acholyn requested a review from tcouch January 30, 2023 12:20
@acholyn acholyn mentioned this pull request Jan 30, 2023
2 tasks
@tcouch
Copy link
Collaborator

tcouch commented Feb 2, 2023

A few notes on the current state of the Work detail page:

  • Unknown book shouldn't have the up/down buttons since it should always be at the bottom (you get an error when clicking these at the moment)
  • Reordering books needs to trigger reindexing wrt work so the Fragment names get update
  • If you look at /work/22/ for example, the Apposita sub headings appear the same size as the book headings.
  • On the Antiquarian detail pages, it looks like Anonymous Fragment links don't have a possible/definite indicator

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.

Just gone through the migration first, but I've noticed a few things that could do with looking at/fixing

src/rard/research/migrations/0061_order_in_book.py Outdated Show resolved Hide resolved
src/rard/research/migrations/0061_order_in_book.py Outdated Show resolved Hide resolved
src/rard/research/migrations/0061_order_in_book.py Outdated Show resolved Hide resolved
src/rard/research/migrations/0061_order_in_book.py Outdated Show resolved Hide resolved
src/rard/research/migrations/0061_order_in_book.py Outdated Show resolved Hide resolved
src/rard/research/migrations/0061_order_in_book.py Outdated Show resolved Hide resolved
src/rard/research/migrations/0061_order_in_book.py Outdated Show resolved Hide resolved
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.

Some fixes for these are included in the following PR.

Comment on lines 619 to 634
# class InlineFragmentForm(forms.ModelForm):
# class Meta:
# model = antiquarian
# fields = ("book", "work")


# FragmentInlineUpdateFormset = inlineformset_factory(
# Book,
# Work,
# form=InlineFragmentForm,
# fields=("book", "work"),
# labels={"book": "Book", "work": "Work"},
# can_delete=False,
# )


Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this go?

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 believe that's the basis of the inline form for updating the links, so I can do since we'll be covering it in the other PR

src/rard/research/models/work.py Outdated Show resolved Hide resolved
src/rard/templates/base.html Show resolved Hide resolved
Comment on lines +209 to +214
{% if perms.research.view_fragment %}
<a class='nav-link {% if "unlinked_fragment" in request.resolver_match.namespaces %}active font-weight-bold{% endif %}'
href='{% url "unlinked_fragment:list" %}'>
{% trans 'Unlinked Fragments' %}
</a>
{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good addition. Should really be in a separate pull request.

Comment on lines +1 to +10
{% load crispy_forms_tags %}
{% if can_edit %}

{% comment %} selects contain the books and works of the antiquarian and will work by displaying the work first and then display the books in that work {% endcomment %}
<div>
<select>
</select>

</div>

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is work in progress, best not to submit it in this PR.

src/rard/research/tests/models/test_antiquarian.py Outdated Show resolved Hide resolved
@@ -756,6 +773,9 @@ def test_testimonium_queryset_methods(self):
self.assertEqual(0, self.testimonium.definite_antiquarian_links().count())
self.assertEqual(1, self.testimonium.possible_antiquarian_links().count())

@pytest.mark.skip(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably best to have this working so that we remember to investigate if work on changes to definite status break it.

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 fixed it in the new feature. I can work on fixing it for this PR but will need to rebuild the db and everything

@@ -316,6 +316,7 @@ def test_link_antiquarian_orders_sequentially(self):
):
self.assertEqual(count, link.order)

@pytest.mark.skip("the opposite of this would be true now")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Testimonia are a special case where those belonging to unknown work should be ordered first. Antiquarian reindex method needs updating to fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah...I'll need to fix that in the new feature as unknown is at the end I believe

@acholyn acholyn marked this pull request as ready for review April 25, 2023 10:01
@acholyn acholyn merged commit de8fe63 into development Apr 25, 2023
@acholyn acholyn deleted the fix/fragment-ordering branch April 25, 2023 10:02
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.

None yet

2 participants