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/item to collection admin enhancement #151

Conversation

lukehollis
Copy link
Contributor

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:
#129
#90
#87

As well as the Collection UI mentioned in this issue:
#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
Copy link
Contributor

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

@lukehollis lukehollis force-pushed the feature/item-to-collection-admin-enhancement branch from 0537bd8 to bc54dfa Compare March 25, 2021 17:22
@lukehollis
Copy link
Contributor Author

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

@rsinghal
Copy link
Contributor

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

templates/pages/full.html Show resolved Hide resolved
@npicardo
Copy link
Collaborator

npicardo commented Mar 31, 2021

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

@npicardo
Copy link
Collaborator

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
Copy link
Collaborator

npicardo commented Mar 31, 2021

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@lukehollis
Copy link
Contributor Author

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: #157

@npicardo
Copy link
Collaborator

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
Copy link
Contributor Author

Confirming that this includes a commit to address #157 now.

@rsinghal
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

@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.

@rsinghal rsinghal merged commit 2a84e83 into artshumrc:master Jul 8, 2021
This pull request was closed.
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.

4 participants