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

43 events gallery of photos #203

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from
Open

Conversation

AvilaAndre
Copy link
Contributor

Closes #43

Creates the endpoints to manage events' photos.

Review checklist

  • Behavior is as expected
  • Clean, well structured code
  • Properly documents API changes in the corresponding controller test
  • Contains enough appropriate tests
  • Changes are reflected in the Wiki if necessary

@AvilaAndre AvilaAndre linked an issue Nov 21, 2023 that may be closed by this pull request
@MRita443
Copy link
Collaborator

Would it be possible to generalize the gallery to activity? We were discussing this today and think that projects benefit from a gallery too for all the images in their details page

@AvilaAndre
Copy link
Contributor Author

Would it be possible to generalize the gallery to activity? We were discussing this today and think that projects benefit from a gallery too for all the images in their details page

Of course! I'll get onto it :)

@AvilaAndre AvilaAndre marked this pull request as ready for review February 6, 2024 14:23
Copy link

github-actions bot commented Feb 6, 2024

Check the documentation preview: https://65c247bae925ce12954d3af9--niaefeup-backend-docs.netlify.app

@MRita443
Copy link
Collaborator

Hi @AvilaAndre are the alterations now ready?

Copy link
Collaborator

@MRita443 MRita443 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few suggestions so far, will look at the tests after these changes :)

@@ -74,4 +74,18 @@ class EventController(private val service: EventService) {
@PathVariable idEvent: Long,
@PathVariable idAccount: Long
) = service.removeTeamMemberById(idEvent, idAccount)

@PutMapping("/{idEvent}/gallery/addPhoto", consumes = ["multipart/form-data"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sirze01 Could you verify the restfulness of these endpoints ;P?

Copy link

Check the documentation preview: https://660614c8b18ed86d0f608ab8--niaefeup-backend-docs.netlify.app

Copy link
Member

@BrunoRosendo BrunoRosendo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job, this is looking good! I got a few suggestions

Comment on lines 93 to 97
if (activity is Event) {
imageFolder = EventService.IMAGE_FOLDER
} else if (activity is Project) {
imageFolder = ProjectService.IMAGE_FOLDER
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a big violation of OOP and we can improve this. My suggestion is to a method like this in the abstract activity service:

Suggested change
if (activity is Event) {
imageFolder = EventService.IMAGE_FOLDER
} else if (activity is Project) {
imageFolder = ProjectService.IMAGE_FOLDER
}
fun addGalleryImage(activityId: Long, image: MultipartFile, imageFolder: String): Activity {
val activity = getActivityById(activityId)
val fileName = fileUploader.buildFileName(image, activity.title)
val imageName = fileUploader.uploadImage(imageFolder + "/gallery", fileName, image.bytes)
activity.gallery.add(imageName)
return repository.save(activity)
}

And then add new methods to the project/events services where the name is chosen. This would maintain the logic and decouple the inheritance logic from the abstract service

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the highlighted code as proposed, while keeping the companion objects.

@@ -271,6 +274,89 @@ internal class EventControllerTest @Autowired constructor(
}
}

@NestedTest
@DisplayName("GET events/category/{category}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this not tested before? I'm a bit confused since it seems out of scope for this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't remember this part, my guess is that I updated the branch and needed to modify it to include the gallery.

jsonPath("$.description").value(testEvent.description),
jsonPath("$.teamMembers.length()").value(testEvent.teamMembers.size),
jsonPath("$.gallery.length()").value(1),
jsonPath("$.gallery[0]").value(expectedImagePath),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you (manually) tested if you can access this path on our machine (e.g. open in your browser)? I assume this was already tested for other images but I don't fully remember

Copy link
Contributor Author

@AvilaAndre AvilaAndre Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It exists on the filesystem but the url does not work (I don't know if I am missing something because the port is 3000 and there is nothing there).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check #71 again, I'm pretty sure @DoStini already had the logic for this. There is a folder where Spring will save public files (see https://github.com/NIAEFEUP/website-niaefeup-backend/wiki/File-Upload#static-file-uploader)

I suspect the servePath is not being used correctly

Copy link

Check the documentation preview: https://669c45f2f8996dd696c64902--niaefeup-backend-docs.netlify.app

Copy link

Check the documentation preview: https://669c47c0b2a594ddcc9e90da--niaefeup-backend-docs.netlify.app

Copy link

Check the documentation preview: https://669c5eda12f07f61dbb35aea--niaefeup-backend-docs.netlify.app

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.

events: gallery of photos
3 participants