-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature/item to collection admin enhancement #151
Conversation
This will need to be rebased after #150 is merged to remove the first two commits |
…Elasticsearch and add frontend for general public user to create Collections and add and remove items
0537bd8
to
bc54dfa
Compare
Okay, rebased like the last PR from the latest on |
Thanks, please move related issues to Needs Review so the team knows what to check |
When public users create a MyGiza Collection, it should be private by default, not viewable by everyone. @lukehollis |
On record view pages (/full/) , the “Add this to a collection” looks good for users to add an image to a Collection. |
Collection display page appears to limit number to max 20 images with no way to advance to those beyond 20 count. |
Is a Mirador window envisioned as viewer for MyGiza Collections display page? |
Also noticed the facets need some fixing: https://giza-web2.rc.fas.harvard.edu/collections/serving-statues |
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? |
… on all collections page
Confirming that this includes a commit to address #157 now. |
@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 |
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:
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:
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:I also took the liberty of adding a Remove button that you can change the look and placement of as need be: