cmu-lib / archive_plugin

Janeway plugin for managing journal archiving.
MIT License
8 stars 2 forks source link

new Archive model #21

Closed mdlincoln closed 5 years ago

mdlincoln commented 5 years ago

This PR does two main things:

  1. Introduces an Archive class that, at the moment, is just a 1-to-1 model attached to the Issue class. The presence of an Archive instance for an Issue indicates that issue instance is an archive.
    1. Adds a step to the create_archive command that creates a new Archive instance for the newly-created Issue
    2. Changes views and hooks to now determine if a Version is "archived" based on the existence of an Archive instance for an article's Issues
  2. Updates field definitions for the Version class
    1. Removes unused boolean fields for changed author & is_archived, and removes a "date updated" field. Replaces these with class methods since they are all computable from existing DB fields.
    2. Changes foreign key on_delete methods for Versions to ensure DB consistency. No application code allows you to delete articles right now, but it is currently possible to delete them from the admin interface, and this causes unexpected behavior with the archive plugin if Article relationships are merely set to None when an article is removed.
      • If an article is deleted, it's associated Version will also be removed.
      • If you attempt to remove an article that serves as an immediate parent article for a version, this raises a PROTECT error
      • If you attempt to remove an article that serves as a base article for any version, this raises a PROTECT error

Some questions:

hachacha commented 5 years ago

In order for the article_version_list.html template to know if an article has been archived, I pass the list of archived versions and have the template do the comparison. This feels like it puts a bit too much logic into the template, but maybe that's unavoidable? What do you think?

I think that the only other solution other than doing that logic in the template is to add a field to Version to keep track of any issue that it's added to through the create_archive command.

if is_latest:
    new_issue.articles.add(article)
    v = Version.objects.get(article=article)
    v.archived_issues.add(new_issue)

and then when returning in the article_version_list {% for issue in version.archived_issues.all %} . Do you see any problems with this? Is this against DRY principles?

This doesn't yet build any hook to let you edit an issue to turn it into an archive / remove it's archive status. (although maybe you shouldn't be able to manually pick and choose what articles get listed in an archive edition, as that seems to miss the point of being a snapshot of the whole journal at a state in time?)

Yeah, that's a good question... It could let people use this archive plugin in different ways. We are coming in to this with specific context for ETHOS where it is a snapshot of a whole thing. But others may think of an archive differently. I don't think we need to add this now.

For ETHOS, if Chris wants a display of normal, non-archive issues, we'd need to build that out in the templates cause they aren't anywhere now. That's sort of a new feature not in the original spec. Might impact the way you implement the archive-friendly search, lmk how you'd like to coordinate that.

kk. will have to talk to Chris about that because it's likely a design thing. Should they show up on the TOC page? They could certainly still show up in "Featured Issue" or "Current Issue" as a homepage element.

mdlincoln commented 5 years ago

I think that the only other solution other than doing that logic in the template is to add a field to Version to keep track of any issue that it's added to through the create_archive command.

😂 no no no, this PR got RID of that is_archived field! Just say no to database inconsistency 🗡 Really what I was trying to do was use Django's annotate() to check whether an article's issues had any Archives and compute that boolean field on the fly. I was having difficulty getting it work though, so I put in this slightly kludgier bit.

mdlincoln commented 5 years ago

kk. will have to talk to Chris about that because it's likely a design thing. Should they show up on the TOC page? They could certainly still show up in "Featured Issue" or "Current Issue" as a homepage element.

Good ? but also a feature request, so I'd log it as new work to be done. Chris has never specified what that'd look like, if it'd be a separate page from the archives, what function a non-archive issue would fill with ETHOS, or any of that.

mdlincoln commented 5 years ago

@hachacha success! Figured out how the annotate function works - apparently you define a subquery and can then pass it to the real query and check whether any Archives come up for a given article version