Code review

src/templates/src/forum_category.html

@@ -36,6 +36,7 @@
 <div class="options">
     {{ if .User }}
         <a class="button new-thread" href="{{ printf "%s/t/new" .CategoryUrl }}"><span class="big">+</span> New Thread</a>
+		{{/* TODO(asaf): Mark read should probably be a POST, since it's destructive and we would probably want CSRF for it */}}
         <a class="button" href="{{ printf "%s/markread" .CategoryUrl }}"><span class="big">&#x2713;</span> Mark threads here as read</a>
     {{ else }}
         <a class="button" href="{% url 'member_login' subdomain=request.subdomain %}">Log in to post a new thread</a>

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,6 +143,7 @@ func ForumCategory(c *RequestContext) ResponseData {
 
 		return templates.ThreadListItem{
 			Title: row.Thread.Title,
+			// 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),
@@ -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{

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))
 }
 

src/website/routes.go

@@ -73,6 +73,7 @@ func NewWebsiteRoutes(conn *pgxpool.Pool, perfCollector *perf.PerfCollector) htt
 	})
 	mainRoutes.GET(`^/feed(/(?P<page>.+)?)?$`, Feed)
 
+	// TODO(asaf): Trailing slashes break these
 	mainRoutes.GET(`^/(?P<cats>forums(/[^\d]+?)*)/t/(?P<threadid>\d+)(/(?P<page>\d+))?$`, ForumThread)
 	// mainRoutes.GET(`^/(?P<cats>forums(/cat)*)/t/(?P<threadid>\d+)/p/(?P<postid>\d+)$`, ForumPost)
 	mainRoutes.GET(`^/(?P<cats>forums(/[^\d]+?)*)(/(?P<page>\d+))?$`, ForumCategory)