artshumrc / giza

JSON API (for TMS Database) and Django 2 application for Digital Giza
http://giza.fas.harvard.edu/
7 stars 5 forks source link

Feature/item to collection admin enhancement #151

Closed lukehollis closed 3 years ago

lukehollis commented 3 years ago

This is a bigger PR to address integrating Elasticsearch Items <> Collections in a more robust way. The same underlying structure now should address these three issues: https://github.com/artshumrc/giza/issues/129 https://github.com/artshumrc/giza/issues/90 https://github.com/artshumrc/giza/issues/87

As well as the Collection UI mentioned in this issue: https://github.com/artshumrc/giza/issues/94

From email @ColeDCrawford mentions: """ You should be storing a composite key for the references to items in the collection. Django doesn't support this out of the box, but you can add a composite field type that abstracts it for you: https://pypi.org/project/django-composite-field/. That way you can just access the values (eg item.id and item.type) and you don't have to do any messy string manipulation (Django 2.2 doesn't support JSONField which is the other way you could go). Since all we care about is storing the reference to this external Elasticsearch data, I would store these as a list in an ArrayField. If we start storing more data (eg when an item is added to the collection, or who added it if the collection can be edited by multiple users) than you should go the typical relational database normalization route - create a new model for a CollectionItem and store everything in there as fields. That's what my approach for storing the data for #87 and #90 would be. """

In practice, I created a dev branch that used the django-composite-field package and ran into issues with it interfering with ArrayField. These are two of the errors that I received both on the admin backend--it seems like ArrayField expects a number of addition methods to the CompositeField custom field that weren't there, and even when I stubbed out these methods, it wasn't able to display a form properly. I haven't worked with CompositeField in the past, so anyone that's more familiar with CompositeField may be able to work around these:

Screen Shot 2021-03-13 at 8 51 07 PM Screen Shot 2021-03-13 at 9 09 57 PM

with code for your review at https://github.com/lukehollis/giza/blob/e1fc2bea6174c32f746ba62c07d106ce6020361d/giza/models.py#L18

At the end of your email @ColeDCrawford you mention """ Feel free to take ownership of the development process and come up with a solution that works though if you have other ideas. But I think that storing all of these composite item references as delimited string in a single field is a bad idea that will be hard to maintain and validate. """ So I went back to what I'd used in practice in other projects successfully in the past with using a relationship as currently is implemented in the PR here: https://github.com/lukehollis/giza/blob/feature/item-to-collection-admin-enhancement/giza/models.py#L101

For what it lacks in the fanciness of the CompositeField, I'm hoping it makes up for being as vanilla of Django as we could implement for this solution.

What it looks like on the Admin backend then is this: Mar-14-2021 21-56-10

And if you're a general public user, now there's a link in the full.html file as marked up by Heather that toggles a modal window: Mar-14-2021 21-58-34

I also took the liberty of adding a Remove button that you can change the look and placement of as need be: Mar-14-2021 21-59-46

rsinghal commented 3 years ago

This will need to be rebased after #150 is merged to remove the first two commits

lukehollis commented 3 years ago

Okay, rebased like the last PR from the latest on master, @rsinghal, when you're ready

rsinghal commented 3 years ago

Thanks, please move related issues to Needs Review so the team knows what to check

npicardo commented 3 years ago

When public users create a MyGiza Collection, it should be private by default, not viewable by everyone. @lukehollis

npicardo commented 3 years ago

On record view pages (/full/) , the “Add this to a collection” looks good for users to add an image to a Collection.
BUT, currently the button adds only the primary rendition. If I select another image available in the Mirador window, clicking “add” still registers the primary rendition over the one active in the viewer and adds nothing to the Collection. I think this blocks the majority of media from inclusion in Collections. @lukehollis @rsinghal @ColeDCrawford

npicardo commented 3 years ago

Collection display page appears to limit number to max 20 images with no way to advance to those beyond 20 count.
Example: https://giza-web2.rc.fas.harvard.edu/collections/serving-statues (29 images in Collection and aggregate count in panel at left, but 20 displayed and 20 listed in collection items count). @lukehollis @rsinghal @ColeDCrawford

npicardo commented 3 years ago

Is a Mirador window envisioned as viewer for MyGiza Collections display page?
e.g. https://giza-web2.rc.fas.harvard.edu/collections/serving-statues @lukehollis @rsinghal @ColeDCrawford

rsinghal commented 3 years ago

Also noticed the facets need some fixing: https://giza-web2.rc.fas.harvard.edu/collections/serving-statues

Screen Shot 2021-03-31 at 4 52 44 PM

lukehollis commented 3 years ago

Sounds good with the facets--what should be the expected behavior with Facets on Collections?

lukehollis commented 3 years ago

For managing public/private from @npicardo's comment, I wrote up a potential solution in this issue that I can implement if it seems correct: https://github.com/artshumrc/giza/issues/157

npicardo commented 3 years ago

Sounds good with the facets--what should be the expected behavior with Facets on Collections?

Would it make sense to use the same facet selection process as those referred to in Issue #138?

lukehollis commented 3 years ago

Confirming that this includes a commit to address https://github.com/artshumrc/giza/issues/157 now.

rsinghal commented 3 years ago

@lukehollis private collections are still publicly accessible and users can't see their own private collections.

https://giza-web2.rc.fas.harvard.edu/collections/user A non authenticated user can go to this link and see private collections, such as Cole's: https://giza-web2.rc.fas.harvard.edu/collections/cole-test-collection-tomb-chapels-and-shafts

Cole cannot see his own private collection when he is logged in and goes to https://giza-web2.rc.fas.harvard.edu/collections/user.

ColeDCrawford commented 3 years ago

As a non-admin user (no Django admin access), I get the option to create a collection even though the text above says that’s coming soon. This throws an error if I try to create a collection.

Screen Shot 2021-04-23 at 11 58 30 AM Screen Shot 2021-04-23 at 11 58 36 AM
ColeDCrawford commented 3 years ago

@lukehollis private collections are still publicly accessible and users can't see their own private collections.

https://giza-web2.rc.fas.harvard.edu/collections/user A non authenticated user can go to this link and see private collections, such as Cole's: https://giza-web2.rc.fas.harvard.edu/collections/cole-test-collection-tomb-chapels-and-shafts

Cole cannot see his own private collection when he is logged in and goes to https://giza-web2.rc.fas.harvard.edu/collections/user.

The flag for public/private works for whether a collection appears at https://giza-web2.rc.fas.harvard.edu/collections/. It does not do anything for whether a user can access the collection. And as Rashmi mentioned, the logic for /collections/user seems backward - anyone can see my private collections there, but I can't see them. An unauthenticated user trying to hit /collections/user should either be prompted to sign in or just redirected back to the public collections page.