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

Image endpoints #64

Merged

Conversation

AlDu2407
Copy link
Contributor

GET endpoint:

  • Added new handler for fetching previously uploaded images from Urchin.
  • Added structs to allow handling of responses with compiler support.
  • Added required functions for the database to fetch images.

DELETE endpoint:

  • Added new handler for deleting previously uploaded images from Urchin.
  • Added required functions for the database to delete images.

General update:

  • Added structs to allow handling of responses with compiler support.
  • Restructured API to used path variables for the delete endpoints.

AlDu2407 and others added 2 commits March 24, 2024 17:35
`GET` endpoint:
- Added new handler for fetching previously uploaded images from Urchin.
- Added structs to allow handling of responses with compiler support.
- Added required functions for the database to fetch images.

`DELETE` endpoint:
- Added new handler for deleting previously uploaded images from Urchin.
- Added required functions for the database to delete images.

General update:
- Added structs to allow handling of responses with compiler support.
- Restructured API to used path variables for the delete endpoints.
Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Attention: Patch coverage is 37.60684% with 146 lines in your changes are missing coverage. Please review.

Project coverage is 41.09%. Comparing base (cf04c2f) to head (d8a25ab).

Files Patch % Lines
app/image.go 1.36% 71 Missing and 1 partial ⚠️
database/database.go 0.00% 39 Missing ⚠️
admin-app/image.go 72.13% 14 Missing and 3 partials ⚠️
admin-app/post.go 36.00% 16 Missing ⚠️
tests/mocks/mocks.go 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
- Coverage   41.68%   41.09%   -0.60%     
==========================================
  Files          15       18       +3     
  Lines         758      876     +118     
==========================================
+ Hits          316      360      +44     
- Misses        404      476      +72     
- Partials       38       40       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@matheusgomes28 matheusgomes28 left a comment

Choose a reason for hiding this comment

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

This is really good Al, thank you for the feature! I will suggest: let's move the current GET /image endpoint to the main app (non-admin), and for the admin we can simply return the image metadata (i.e. the Image struct filled with the DB data).

Remember: the admin-app does not need to be running for the app to work, and the file serving should definitely come from the main app 😄

migrations/20240323203547_add_image_ext.sql Outdated Show resolved Hide resolved
admin-app/image.go Outdated Show resolved Hide resolved
admin-app/image.go Outdated Show resolved Hide resolved
admin-app/post.go Outdated Show resolved Hide resolved
common/bindings.go Show resolved Hide resolved
}()

// TODO: We have to create the image first. Maybe there's a better way to do this?
req, _ := http.NewRequest("POST", "/images", pr)
Copy link
Owner

Choose a reason for hiding this comment

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

There is another way: we could mock the access to the image files. I'm thinking of doing something like a MediaVault struct, then that could have AddImage(...), GetImage(...) etc... But this is fine for this PR. I'll create some tasks for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be moved to a separate task. For now, I wouldn't want to increase the size of this PR further more.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes

@matheusgomes28
Copy link
Owner

Issues spawned from this work are #65 and #66

- Updated `GET` image endpoint for admin app.
- Added `GET` images and image endpoints for app.
- Added database functions for newly added app features.
- Added new views for image and images.
- Refactored navigation to simplify configuring navbar.
  - You can define all the `links` in `views/links.go` and they will be
    used for rendering the navbar.
- Minor refactorings to existing code.
@AlDu2407
Copy link
Contributor Author

I added the required functionality to the app. Also, refactored the GET image endpoint in the admin app.
Improved the handling of the NavBar.

Hope this is fine for you.

Copy link
Owner

@matheusgomes28 matheusgomes28 left a comment

Choose a reason for hiding this comment

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

I like the web pages for the image & image gallery to showcase the added images.

I'll take another look and test it all tomorrow morning, for some reason I'm having issues connecting to the docker composed mariadb so I can do a few e2e tests on this.

Sorry to delay this Al, but I just wanna do a quick usability test before we merge!

@@ -34,7 +34,7 @@ func TestPostSuccess(t *testing.T) {

r := app.SetupRoutes(app_settings, database_mock)
w := httptest.NewRecorder()
req, _ := http.NewRequest("GET", "/post/0", nil)
req, _ := http.NewRequest("GET", "/post/1", nil)
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this necessary?

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 have no clue, but for some reason 0 as an ID makes the test fail.

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 tried to take a look into it and I assume that 0 as an ID is interpreted as empty/nil and therefore the required part of the URI binding is not fulfilled. Might be a quirk from the Gin Framework and using their ShouldBindUri(...) function.

Copy link
Owner

Choose a reason for hiding this comment

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

Weird, I'll investigate this. Let's slide this in for now.

Copy link
Owner

@matheusgomes28 matheusgomes28 left a comment

Choose a reason for hiding this comment

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

I was finally able to test this properly. Turns out I was running docker compose up mariadb which is not the same as docker compose -f mariadb.yml up -d ...

Looks fine, let's merge this in then we can perhaps weed out some of the minor stuff later. The things I particularly don't want to keep in the long run are:

  • Absolute sizes for the margins and other CSS stuff - doesn't play well with different resolutions.
  • Fixed links being returned in the GetUrchinLinks - this would be good to get programatically from the user somehow (let them provide nav links). I'll create a feature task for this. Feature to let users specify links to Navbar #70
  • Weird failure when you have the post mock id set to 0, it seemed to be passing in the previous pipelines so I gotta figure out what's going on.
  • Generally remove the handwritten CSS - I dabbled with tailwind before but unfortunately I'm not a very good designer, so let's hope I can find someone to make this all look nicer ! Adding basic tailwind conversion code #10
  • Some system tests for this feature (test real DB code) Add system tests for the image upload endpoint #69

@matheusgomes28 matheusgomes28 merged commit 5372b61 into matheusgomes28:main Mar 29, 2024
3 of 5 checks passed
@AlDu2407 AlDu2407 deleted the feature/get_image_endpoint branch April 1, 2024 01:17
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.

2 participants