From 2d07f57c1a37563b0833f81ec289d7503f319b84 Mon Sep 17 00:00:00 2001 From: Asaf Gartner Date: Tue, 4 May 2021 15:02:33 +0300 Subject: [PATCH] Code review --- src/templates/src/forum_category.html | 3 ++- src/website/forums.go | 17 ++++++++++++++++- src/website/requesthandling.go | 2 ++ src/website/routes.go | 1 + 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/templates/src/forum_category.html b/src/templates/src/forum_category.html index cf5dc121..d98ee9b3 100644 --- a/src/templates/src/forum_category.html +++ b/src/templates/src/forum_category.html @@ -36,6 +36,7 @@
{{ if .User }} + New Thread + {{/* TODO(asaf): Mark read should probably be a POST, since it's destructive and we would probably want CSRF for it */}} ✓ Mark threads here as read {{ else }} Log in to post a new thread @@ -44,4 +45,4 @@
{{ template "pagination.html" .Pagination }}
-{{ end }} \ No newline at end of file +{{ end }} diff --git a/src/website/forums.go b/src/website/forums.go index b8c6e911..501b46ab 100644 --- a/src/website/forums.go +++ b/src/website/forums.go @@ -37,9 +37,18 @@ type forumSubcategoryData struct { func ForumCategory(c *RequestContext) ResponseData { const threadsPerPage = 25 + // TODO(asaf): Consider making this more robust. + // Right now this code allows for weird urls like: + // "/forums/asdf/wip" which doesn't verify the lineage and displays the wip forums + // "/forums/wip///" which fetches the main forums page because it happens to have a blank slug + // "/forums/wip/" which fetches the main forums page because Split returns an extra blank string + // "/forums/wip/1" this one fetches the wip forums because the regex matches the `/1` as part of the page group + // "/forums/wip/1/" 404 - doesn't match the regex + // "/forums/" 404 - doesn't match the regex catPath := c.PathParams["cats"] catSlugs := strings.Split(catPath, "/") currentCatId := fetchCatIdFromSlugs(c.Context(), c.Conn, catSlugs, c.CurrentProject.ID) + // TODO(asaf): 404 if we can't find our cat. categoryUrls := GetProjectCategoryUrls(c.Context(), c.Conn, c.CurrentProject.ID) c.Perf.StartBlock("SQL", "Fetch count of page threads") @@ -134,7 +143,8 @@ func ForumCategory(c *RequestContext) ResponseData { return templates.ThreadListItem{ Title: row.Thread.Title, - Url: ThreadUrl(row.Thread, models.CatKindForum, categoryUrls[currentCatId]), + // TODO(asaf): Use thread.category_id instead of currentCatId. At the moment this is generating wrong urls for threads in subcats. + Url: ThreadUrl(row.Thread, models.CatKindForum, categoryUrls[currentCatId]), FirstUser: templates.UserToTemplate(row.FirstUser), FirstDate: row.FirstPost.PostDate, @@ -181,6 +191,7 @@ func ForumCategory(c *RequestContext) ResponseData { catRow := irow.(*subcatQueryResult) c.Perf.StartBlock("SQL", "Fetch count of subcategory threads") + // TODO(asaf): [PERF] [MINOR] Consider replacing querying count per subcat with a single query for all cats with GROUP BY. numThreads, err := db.QueryInt(c.Context(), c.Conn, ` SELECT COUNT(*) @@ -197,6 +208,7 @@ func ForumCategory(c *RequestContext) ResponseData { c.Perf.EndBlock() c.Perf.StartBlock("SQL", "Fetch subcategory threads") + // TODO(asaf): [PERF] [MINOR] Consider batching these. itThreads, err := db.Query(c.Context(), c.Conn, threadQueryResult{}, ` SELECT $columns @@ -293,6 +305,7 @@ type forumThreadData struct { func ForumThread(c *RequestContext) ResponseData { const postsPerPage = 15 + // TODO(asaf): Verify that the requested thread is not deleted, and only fetch non-deleted posts. threadId, err := strconv.Atoi(c.PathParams["threadid"]) if err != nil { @@ -365,6 +378,8 @@ func ForumThread(c *RequestContext) ResponseData { } baseData := getBaseData(c) + // TODO(asaf): Replace page title with thread title + // TODO(asaf): Set breadcrumbs var res ResponseData err = res.WriteTemplate("forum_thread.html", forumThreadData{ diff --git a/src/website/requesthandling.go b/src/website/requesthandling.go index 6575329d..e2fd49c8 100644 --- a/src/website/requesthandling.go +++ b/src/website/requesthandling.go @@ -107,6 +107,8 @@ func (r *Router) ServeHTTP(rw http.ResponseWriter, req *http.Request) { return } + // TODO(asaf): Replace this with a 404 handler? Isn't this going to crash the server? + // We're doing panic recovery in doRequest, but not here. I don't think we should have this line in production. panic(fmt.Sprintf("Path '%s' did not match any routes! Make sure to register a wildcard route to act as a 404.", path)) } diff --git a/src/website/routes.go b/src/website/routes.go index 5c3650bf..dc3ce5e4 100644 --- a/src/website/routes.go +++ b/src/website/routes.go @@ -73,6 +73,7 @@ func NewWebsiteRoutes(conn *pgxpool.Pool, perfCollector *perf.PerfCollector) htt }) mainRoutes.GET(`^/feed(/(?P.+)?)?$`, Feed) + // TODO(asaf): Trailing slashes break these mainRoutes.GET(`^/(?Pforums(/[^\d]+?)*)/t/(?P\d+)(/(?P\d+))?$`, ForumThread) // mainRoutes.GET(`^/(?Pforums(/cat)*)/t/(?P\d+)/p/(?P\d+)$`, ForumPost) mainRoutes.GET(`^/(?Pforums(/[^\d]+?)*)(/(?P\d+))?$`, ForumCategory)