Add Pagination #44 #50
Conversation
|
Thanks @ali-assar I'll take a look after work today! Note that the pipeline is failing, think you need to run |
There was a problem hiding this comment.
Nice changes, however they don't completely work when I tried out. I've left some comments 😄
Were you able to run the project btw @ali-assar ? Let me know if you had issues running, I could do with feedback on instructions
app/app.go
Outdated
| @@ -29,6 +30,11 @@ func SetupRoutes(app_settings common.AppSettings, database database.Database) *g | |||
| // DO not cache as it needs to handlenew form values | |||
| r.POST("/contact-send", makeContactFormHandler()) | |||
|
|
|||
| // this will setup the route for pagination | |||
| r.GET("/page/:num", func(c *gin.Context) { | |||
There was a problem hiding this comment.
Let's add this endpoint as a cache-able one here. You can do something simlar to that's above in this file:
addCachableHandler(r, "GET", "/page/:num", homeHandler, &cache, app_settings, database)Also, calling the homeHandler(c, app_settings, database) here won't actually serve the HTML contents, because this function will only return the HTML buffer. Adding it with the cache wrapper will take care of that for you.
app/app.go
Outdated
| @@ -75,7 +81,16 @@ func addCachableHandler(e *gin.Engine, method string, endpoint string, generator | |||
| // / This function will act as the handler for | |||
| // / the home page | |||
| func homeHandler(c *gin.Context, settings common.AppSettings, db database.Database) ([]byte, error) { | |||
| posts, err := db.GetPosts() | |||
| pageNumQuery := c.Param("num") | |||
There was a problem hiding this comment.
a few issues with these additions. Firstly, this is user data, so this can be a negative number or empty, hence the Atoi(pageNumQuery) will fail.
I would personally do something like
pageNum := 0
if pageNumQuery := c.Param("num"); pageNumQuery != "" {
num, err := strconv.Atoi(pageNumQuery)
if err == nil {
pageNum = num
} else {
log.Error().Msgf("invalid page number: %d", num)
}
}
app/app.go
Outdated
| } | ||
|
|
||
| limit := 10 // or whatever limit you want | ||
| offset := (pageNum - 1) * limit |
There was a problem hiding this comment.
If pageNum is 0, then this is not gonna be a nice offset! Try something like taking the max of this and zero.
app/app.go
Outdated
| posts, err := db.GetPosts() | ||
| pageNumQuery := c.Param("num") | ||
| pageNum, err := strconv.Atoi(pageNumQuery) | ||
| if err != nil { |
There was a problem hiding this comment.
Since this is also handling the / page (home), then this value is potentially empty and the error will be thrown. So only logging and proceeding with a nice default is probably nicer. See the comment above.
| @@ -11,7 +11,7 @@ import ( | |||
| ) | |||
|
|
|||
| type Database interface { | |||
| GetPosts() ([]common.Post, error) | |||
| GetPosts(int, int) ([]common.Post, error) | |||
There was a problem hiding this comment.
You changed the interface, don't forget to change the tests too! See the cmd/urchin/index_test.go
|
Also, don't forget to run |
|
@ali-assar please don't reopen another PR, just use this one. I can't accept multiple PRs! |
|
thanks for your help I have implemented your wanted changes. Thanks for your help. Now all the test would be working fine. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
==========================================
+ Coverage 23.83% 24.96% +1.12%
==========================================
Files 14 14
Lines 646 661 +15
==========================================
+ Hits 154 165 +11
- Misses 475 477 +2
- Partials 17 19 +2 ☔ View full report in Codecov by Sentry. |
app/app.go
Outdated
| return []byte{}, err | ||
| } | ||
|
|
||
| return html_buffer.Bytes(), nil | ||
| } | ||
|
|
||
| func max(a, b int) int { |
There was a problem hiding this comment.
There's already a function in the standard called max(...) examples here
app/app.go
Outdated
| pageNum = max(pageNum, 1) | ||
|
|
||
| limit := 10 // or whatever limit you want | ||
| offset := (pageNum - 1) * limit |
There was a problem hiding this comment.
just put the max here, remove the pageNum = max(pageNum, 1) above.
offset := max((pageNum - 1) * limit, 0)
|
I see your point so I changed the code as requested. |
|
Thanks for your help I've learn a lot. also i want to use this opportunity and in advance wish you a happy Nowruz which is the Persian New Year. Nowruz marks the beginning of spring and symbolizes renewal, hope, and new beginnings. 🌸🌼 |
|
Thank you for the wishes @ali-assar! Happy Nowruz to you too, my friend. This is the nicest Github message I've received, so thank you! I hope to see more contributions from you here at Urchin, been a pleasure reviewing your code 🤝 |
Here is the things I've done for pagination:
I edited the database query to support pagination
Then I saw home handler use GetPosts so i modified that handler in app.go
and at the end I Changed SetupRoutes in app.go