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/BE-30: Exhibitions #344

Merged
merged 28 commits into from
Dec 19, 2022
Merged

feature/BE-30: Exhibitions #344

merged 28 commits into from
Dec 19, 2022

Conversation

KarahanS
Copy link
Contributor

@KarahanS KarahanS commented Dec 8, 2022

This PR is directly related to #343. Additionally, it involves minor fixes/updates on other parts:

  • Date time format is updated. Older version was something like this: 2022-11-18T13:51:56.342042Z. New version will be something like this: 08-12-2022 00:38:25.
  • Time zone is updated. (It was UTC-0, now it's Istanbul/Europe).
  • Minor fixes on the documentation of art item related APIs.
  • Major change: Implementation of exhibition related APIs:
    • Created four new models: AbstractExhibition, VirtualExhibition, OfflineExhibition, ExhibitionArtItem. AbstractExhibition is an abstract model that doesn't have a correspondence in the database directly. Offline and online exhibitions are derived from it.
    • Implemented all the API endpoints listed in BE-30: [API] Implementation of Exhibition Related APIs - v1 #343. Documented them with Swagger.
    • Implemented unit tests to test helper function I've implemented for the APIs.
  • IMPORTANT:
    • Now, we have APIs for both online and offline exhibitions. Offline exhibitions only require a single image (poster) from the user - it also requests some information about the location such as longitude and latitude. Frontend should provide this information using a library or a geolocation API.
    • For online exhibitions, we don't take any location information. During the creation of an online exhibition, user can add images to it by two means: either via uploading from scratch or from his gallery (previously uploaded images). Previously uploaded images should be provided as a list of IDs in the field artitems_gallery. New images should be provided as list of base64 encodings in the field artitems_upload. During our meetings, we didn't give a final decision at this point, therefore I wanted to provide both of the opportunities to users. (Please keep in mind that uploading more than one image to S3 will take some time - so you may want to add some 'loading' effect or something)
    • Lastly, for now adding collaborators to the exhibition doesn't really mean anything for the online scenario. Because the only time one can upload images to an exhibition is during its creation. I can work on version 2 of exhibition related PRs if we take such requests from frontend and mobile in the future. But, to be realistic, I believe this implementation is more than enough for a majority of the exhibition related requirements we have and it's overkilling to try to implement all those additional features now while we have lots of other works to do in our schedule.)
  • How to test:
    • Code may seem a little bit overwhelming - so you don't really need to analyze it in detail. Best you can do is to test the APIs from your Postman. I have a small Postman collection for exhibition related APIs which can be found here:
      Run in Postman
    • You can have a very brief look at the Swagger documentation to see If I'm missing something. Sometimes I make simple mistakes while writing those docs.
    • I tried to cover edge cases as much as possible. Try APIs with different inputs. If it's working properly and docs seems fine - then please approve. Thanks!

@KarahanS KarahanS added Effort: High Handling this issue might take longer Priority: High This issue must be handled immediately Status: Review Needed A review is needed for this issue Coding The issue is related with coding Team: Backend issues related to backend labels Dec 8, 2022
@KarahanS KarahanS self-assigned this Dec 8, 2022
@KarahanS
Copy link
Contributor Author

In the lab session of 13/12/2022, we talked about sending an additional field from exhibition related APIs that gives the status information of the exhibition. I added the corresponding field:

  • status: On Going (current date is between start date and end dat e)
  • status: Not Yet Started (current date is earlier than the start date)
  • status: Finished (current date is late then the start date)
    Also added a Constraint to check if the given end_date is greater than the start_date. Probably, we won't receive such input from frontend (date selection will be handled accordingly) but I wanted to handle that case as well.
    Updated the documentation accordingly. This PR is still open to review.

@BElifb
Copy link
Contributor

BElifb commented Dec 17, 2022

  • I have been looking into your pull request. Firstly, thanks for all of the detailed explanations and the postman collection.
  • I went through what has changed in terms of commits. There were a few things I felt uneasy about (not necessarily problems).
    • I couldn't see why we would want to create a separate class for exhibition art items, instead of just pulling existing ones using ids.
    • We have separate API end points for get and delete functionality, I recall you mentioning that we should try to avoid relating APIs with actions but rather classes (or objects themselves). Could they be on the same endpoint?
  • I started testing with Postman, using the collection you provided and swagger. I am not done with all of the endpoints, but here are a few remarks;
    • When I tried testing POST APIs with erroneous input (such as end date being earlier than start, which was the case with the collection ones), the API returned an error message. But when I checked art items through the admin site, I realized that even though the API returned an error the art items were created in the database. This sounds like a situation we would't want, so we could either check before creating art items or find a way of reverting in case of an error.
    • I also realized that we were creating and storing posters in art item form. I again don't see the point as a poster doesn't have any art item quality other than the image.
    • And I am truly sorry for all of you who have had to deal with base64 images, didn't know they were this long. I tested POST API for online exhibition with sending existing art item ids, but with creating new art items at online exhibition creation, I get an art item without a name (using encryption provided in collections). A quick demo or a link about base64 would be appreciated.
  • And lastly I feel like collaboration on exhibitions might be one of those pain points, that the professor was talking about. With all of the models and infrastructure in place hopefully it won't be too hard to create an UPDATE API that collaborators can use for adding their art items.
  • I can continue reviewing tomorrow, let me know your preference about possible updates (this PR or another).

… requests, updated exhibition related serializers and updated Swagger
@KarahanS
Copy link
Contributor Author

KarahanS commented Dec 17, 2022

Thank you for the detailed examination @BElifb . Let me list my updates:

  • I created a separate class because I wanted ArtItemExhibition class to have a foreign key to an exhibition. Since not all art items are displayed in exhibitions, I thought that would be better to create a new class. However, I didn't know the fact that foreign keys can be set to null = True and blank = True (meaning that it's not compulsory for an art item to have an associated exhibition although it has such a foreign key field) I realized this feature while dealing with the annotations yesterday. Now, updated ArtItem model accordingly and got rid of ArtItemExhibition model. Btw we can't simply pull existing ones using ids because we also want users to upload art items only for exhibitions. (We could have enforce frontend to first call POST artitem APIs and then POST exhibition API but I wanted them to do everything in one call)
  • I combined GET online exhibition by id and DELETE online exhibition by id together. Did the same for offline exhibitions as well. Update the routes a little bit, I'll be providing a new Postman Collection button below.
  • Nice catch! When I returned an IntegrityError due to the dates, I was not deleting the already saved poster from the database. Now it should be working correctly.
  • Updated the online exhibition creation so that now frontend should also provide title, description and so on for each art item (same as creating an art item). Now, you shouldn't be seeing empty names. Updated the Swagger documentation for the input. If you want to use different base64 images, simply download a one from internet and go to https://www.base64-image.de/, encode it and put it into the input JSON.
  • Added PUT request for online exhibitions. A collaborator can edit title, description, add images via upload, add images via IDs (from gallery), or delete any image in the exhibition.
  • Updated GET requests by user id so that it returns the exhibitions in which the currently logged in user is a collaborator (not an organizer).
  • Still, in order to be able to delete an exhibition, you must be the organizer. A collaborator cannot delete an exhibition.

Now, PR is again open to review. Thank you for your time.
Here is the updated version of APIs:

Run in Postman

@KarahanS KarahanS changed the title feature/BE-30 feature/BE-30: Exhibitions Dec 18, 2022
@BElifb
Copy link
Contributor

BElifb commented Dec 18, 2022

  • Was receiving ImproperlyConfigured: Field name "type" is not valid for model "ArtItem". error, fixed by changing "type" to "category" in class SimpleArtItemSerializer.
  • About VirtualExhibition field of ArtItem model; an art item can be included in multiple exhibitions but this is not a manytomany field. As I understand you are planning on using this to immediately delete art items created for a specific exhibition once that exhibition is deleted. This is not really the case for real life exhibitions as people don't really burn away their paintings even if they were initially created for an exhibition. This was one of the reasons why I was weary about creating art items specifically for exhibitions in the first place. But regardless, users always have the option to manually create an art item and add it to an exhibition (which won't go away). In the spirit of not prolonging the PR, I think this version is ok.
  • When I tried creating an online exhibition I got the following error:
    • { "category": [ "\"poster\" is not a valid choice." ] }
    • I tried with both the collection input body and the one I copied and edited from swagger. Might be giving the parameters wrong.
  • Hence didn't test any of the online exhibition APIs. (including PUT)
  • delete url was wrong in the collection, copied from swagger.
  • Tested all APIs other than online exhibition ones, they all seem to be functioning relly good. Thanks for your efforts.

@KarahanS
Copy link
Contributor Author

KarahanS commented Dec 18, 2022

There were some errors due to the update coming from #358. Now it should be ok.

{ "category": [ ""poster" is not a valid choice." ] }

This error was due to the fact that I had a expression like this: data['category'] = "poster" - I fixed it now.

@KarahanS
Copy link
Contributor Author

About VirtualExhibition field of ArtItem model; an art item can be included in multiple exhibitions but this is not a manytomany field. As I understand you are planning on using this to immediately delete art items created for a specific exhibition once that exhibition is deleted. This is not really the case for real life exhibitions as people don't really burn away their paintings even if they were initially created for an exhibition. This was one of the reasons why I was weary about creating art items specifically for exhibitions in the first place. But regardless, users always have the option to manually create an art item and add it to an exhibition (which won't go away). In the spirit of not prolonging the PR, I think this version is ok.

Yep, this was exactly what I aimed to achieve :D. Frontend can leave a small warning to somewhere stating that artitems that are specifically uploaded for an exhibition will be gone if the exhibition is deleted. Actually, when someone wants to delete an exhibition, this warning can pop up in a very attention drawing manner. Not really important though. As you suggested, a user always can upload an art item to his gallery and then display it in an exhibition. It won't go away when the exhibition is deleted. I don't really think people will want to delete their exhibitions anyways. Why would they

@KarahanS
Copy link
Contributor Author

In #354 we added number_of_views to artitems. Added that field to the Swagger doc. Now it's ready for review again.

@BElifb
Copy link
Contributor

BElifb commented Dec 18, 2022

  • When I tried updating an Exhibition that didn't exist I got the following error. Perhaps we could catch this.

IndexError at /api/v1/exhibitions/online/3

list index out of range
Request Method: PUT
  • When I tried updating partially via following input:
    • { "title": "Updated Art Online" }
    • I got this error:
    • {"detail":"Unsupported media type \"text/plain\" in request."}
  • When I tried again with including parameters but with an empty string such as;
    • { "title": "Updated Art Online", "description": "Updated A collection of beautiful paintings.", "add_via_gallery": [ 9 ], "add_via_upload": [ ], "remove": [ ] }
    • I got the same error
    • {"detail":"Unsupported media type \"text/plain\" in request."}
  • I tested all the APIs. They all worked except PUT.

@KarahanS
Copy link
Contributor Author

Fixed the list index out of range error. I'm not able to reproduce the other errors you mentioned. They work fine for me. Could you try them with the latest version please @BElifb ?

@BElifb
Copy link
Contributor

BElifb commented Dec 18, 2022

  • Okay here's where I'm at after testing the PUT api.
  • I kept getting {"detail":"Unsupported media type \"text/plain\" in request."} error. I played around with other APIs, when I tried login API received the same error. So this was most likely a local issue. It went away when I opened a different window in Postman (still don't understand why, headers and parameters were the same 😕 )
  • When I tried the update API through postman the JSON response returned the updated version but the admin site showed no changes. When I investigated the code realized you forgot to save object after changes, fixed it.
  • Just as a reminder, the try-catch block was too big and I received "Not Found": "There is no virtual exhibition with the given id such that the current user is a collaborator" error when the actual problem was with saving the object. (I accidentally tried saving the serializer initially).
  • "add_via_gallery" didn't work. 😿
  • Haven't tried "add_via_upload"
  • Can confirm "remove" and the other update fields worked.

@BElifb
Copy link
Contributor

BElifb commented Dec 18, 2022

  • Tested "add_via_upload"
  • It seems to be working, art item is created and the exhibitions/online/<id> returns the newly created item.
  • But I it is not visible through the exhibition in the admin site. I though maybe you forgot to add the field to serializer but when I looked into it I realized the VirtualExhibition model doesn't have artitems_upload field at all. Now I'm confused altogether as to how you store newly created art item.

@KarahanS
Copy link
Contributor Author

KarahanS commented Dec 19, 2022

I kept getting {"detail":"Unsupported media type "text/plain" in request."} error. I played around with other APIs, when I tried login API received the same error. So this was most likely a local issue. It went away when I opened a different window in Postman (still don't understand why, headers and parameters were the same 😕 )

I guess you were trying to send the data as Text rather than JSON. Idk, maybe that was the reason.

When I tried the update API through postman the JSON response returned the updated version but the admin site showed no changes. When I investigated the code realized you forgot to save object after changes, fixed it.

Much appreciated!

But I it is not visible through the exhibition in the admin site. I though maybe you forgot to add the field to serializer but when I looked into it I realized the VirtualExhibition model doesn't have artitems_upload field at all. Now I'm confused altogether as to how you store newly created art item.

It is stored as an ArtItem. You can find it among the artitems and you'll see that it has a VirtualExhibition associated with it.

Just as a reminder, the try-catch block was too big and I received "Not Found": "There is no virtual exhibition with the given id such that the current user is a collaborator" error when the actual problem was with saving the object. (I accidentally tried saving the serializer initially).

Updated it, now it gives that error only when such an exhibition cannot be found. I did it intentionally to cover all cases easily but was not a good choice.

"add_via_gallery" didn't work. 😿

Could you please share your input and the expected output? I tried it again and it seems to be working.

Edit: I think I found the problem, could you try it again?

@BElifb
Copy link
Contributor

BElifb commented Dec 19, 2022

  • I gave as input: { "title": "Updated Art Online", "description": "Updated A collection of beautiful paintings.", "add_via_gallery": [ 9 ], "add_via_upload": [ ], "remove": [ ] }
  • And the output was : { "id": 6, "owner": { "id": 3, "username": "string2", "name": "st2", "surname": "ring2", "profile_path": "avatar/default.png" }, "title": "Updated Art Online", "description": "Updated A collection of beautiful paintings.", "poster": { "id": 17, "owner": 3, "title": "Art Online", "description": "A collection of beautiful paintings.", "category": "PT", "tags": [], "artitem_path": "artitem/artitem-14.png", "created_at": "18-12-2022 18:42:10" }, "collaborators": [ { "id": 2, "username": "string", "name": "st", "surname": "ring", "profile_path": "avatar/default.png" } ], "artitems_gallery": [ { "id": 7, "owner": 3, "title": "ArtOfflineexpired", "description": "A collection of beautiful paintings.", "category": "PT", "tags": [], "artitem_path": "artitem/artitem-7.png", "created_at": "18-12-2022 13:04:07" } ], "start_date": "08-12-2022 16:00:00", "end_date": "20-12-2022 16:00:00", "created_at": "18-12-2022 18:42:10", "updated_at": "19-12-2022 12:33:25", "status": "Ongoing", "artitems_upload": [ { "id": 18, "title": "Portrait of Joel Miller", "tags": [], "description": "Joel Miller from TLOU universe.", "category": "OT", "artitem_path": "artitem/artitem-18.png", "likes": 0, "created_at": "19-12-2022 00:52:51" } ] }
  • Going to try updated version now.

@BElifb
Copy link
Contributor

BElifb commented Dec 19, 2022

  • Ok updated version works, confirmed through admin site as well.
  • I think we're finally ready.

@KarahanS KarahanS merged commit dcd4ddb into master Dec 19, 2022
@KarahanS KarahanS deleted the feature/BE-30 branch December 19, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Coding The issue is related with coding Effort: High Handling this issue might take longer Priority: High This issue must be handled immediately Status: Review Needed A review is needed for this issue Team: Backend issues related to backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants