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

Feature/add intro boxes #394

Merged
merged 40 commits into from
Jul 13, 2023
Merged

Feature/add intro boxes #394

merged 40 commits into from
Jul 13, 2023

Conversation

acholyn
Copy link
Collaborator

@acholyn acholyn commented Jun 21, 2023

Summary:
Have added introduction boxes to works and books. This involved changes to the templates, forms, models and views (and migration). Related to #376

Models

  • have added introduction - 1:1 relationship with TextObjectField instance
  • have added plain introduction - plain text version for search purposes
  • this should be the same across ant/work/book
  • when a new work/book is created, it should create an introduction (TOF) object associated with it

Views

  • new update introduction views based on update views- the work one follows the same pattern as the ant one in which it just uses the detail page with functions for handling an update. The book one is more complicated as books have no detail page so it redirects to the creation form but will act as an update. There is a filler placeholder (this is empty) that will create a text object associated with the work/book if it does not already exist

Forms

  • Have taken the existing antiquarian introduction form and created a base form which the ant/work/book introduction forms are based on
  • have added introduction fields to book and work (create) forms

Templates

  • have added the boxes - which just makes use of the preexisting text_object_preview.html partial, same situation for the rich text editors for adding/updating the introduction content
  • have added add/edit buttons depending on if the introduction exists (has not been created yet) and if it is less than a certain length (the content at base has some empty tags - this might be a good idea to update)
  • similarly to above, the boxes showing the introductions should not show if they are empty
  • with the new editing states, they also have tab descriptions

Tests

  • check that TextObjectField instances associated with the work/book introduction are created/deleted with the work/book (models)
  • check that the content is properly updated and that it is in the editing mode (views)

@acholyn acholyn requested a review from tcouch June 21, 2023 15:34

class BookForm(forms.ModelForm):
introduction_text = forms.CharField(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some code repetition here: could you have created a mixin?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a request, just throwing it out there

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 hadn't thought of that - I would like to go through and streamline things at some point actually (there's a fair bit of repetition spread around). Would it be worth opening an issue for? There's one for beautifying the db but that's not quite the same

src/rard/research/models/text_object_field.py Show resolved Hide resolved
src/rard/research/views/work.py Show resolved Hide resolved
self.create_intro_if_does_not_exist()
return super().dispatch(request, *args, **kwargs)

def get_success_url(self, *args, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is almost identical to the get_success_url function in the parent WorkUpdateView class. Is this necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as it turns out, we don't 👍

src/rard/research/views/work.py Show resolved Hide resolved
self.create_intro_if_does_not_exist()
return super().dispatch(request, *args, **kwargs)

def get_success_url(self, *args, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

And, as above, do you need to override this method?

Comment on lines 244 to 247
"change_work": True,
"change_book": True,
"has_object_lock": True,
"instance": self.get_object(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are these for? The only one I can see being used in the template is has_object_lock and usually that's worked out in the template as
{% with request.user|has_lock:object as has_object_lock %}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since the book update redirects to a new page (there's no book detail page) it loses some of the context (like editing permissions)
it's possible that this is no longer the case - I can try removing parts and seeing if it can be slimmed down

@acholyn acholyn merged commit 8478250 into development Jul 13, 2023
@acholyn acholyn deleted the feature/add-intro-boxes branch July 13, 2023 12:21
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.

2 participants