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

feature-friend-request #20

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions resources/migrations/002-friend-requests.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP TABLE friend_requests;
9 changes: 9 additions & 0 deletions resources/migrations/002-friend-requests.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
CREATE TABLE friend_requests (
id SERIAL,
sender_username text,
Copy link
Member

Choose a reason for hiding this comment

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

Use foreign keys to ensure consistency.

recipient_username text,
request_state text,
Copy link
Member

Choose a reason for hiding this comment

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

Consider using postgres Enums for this.

Copy link
Member

Choose a reason for hiding this comment

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

This could just be state

msg text,
creation_timestamp timestamptz,
PRIMARY KEY (id)
);
60 changes: 60 additions & 0 deletions src/chaat/db/friend_request.clj
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
(ns chaat.db.friend-request
(:require
[next.jdbc :as jdbc]
[next.jdbc.sql :as sql]
[chaat.handler.errors :refer [error-table]]
[chaat.db.user :as user]
[java-time.api :as jt]))

;; Will be more logical to have the user-exists? and exists? check in the model layer.
Copy link
Member

Choose a reason for hiding this comment

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

There's a tradeoff here.

  • Putting it in the model layer means you're able to visualize the flow better at the correct layer. Putting it in the db layer lets you use db features to do this better.
  • If you're doing the check in the model layer, you'll have to use a transaction. Otherwise, you'll have to check again in the db layer anyway (or implicitly, it will fail the uniqueness check which you should handle)
  • Just relying on the uniqueness check, will allow you to skip any explicit checks entirely. The downside to this is that it isn't clear from just the code that there is a uniqueness guarantee. However, this may be okay since any complex enough system is relying on the db for a variety of guarantees. It depends on how much you want to tie your app to DB features. The more you rely on them, the more valuable the dependency (but also the more dependent you are and moving away will be harder).

;; Same for the friend request exists? check
(defn exists?
"Check if friend request id exists in friend_requests table"
[connection id]
(not (empty? (sql/find-by-keys connection :friend_requests {:id id}))))

(defn insert
"Insert a friend request into the db"
[db {:keys [sender_username recipient_username] :as content}]
Copy link
Member

Choose a reason for hiding this comment

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

content could be renamed to friend-request

Copy link
Member

Choose a reason for hiding this comment

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

It's usually worth doing the hypen <> underscore translation at the db boundary to make the rest of your clojure code consistent.

(jdbc/with-transaction [tx (db)]
(if (and (user/user-exists? tx sender_username)
(user/user-exists? tx recipient_username))
(let [query-result (sql/insert! tx :friend_requests content)]
Copy link
Member

Choose a reason for hiding this comment

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

Need to handle what happens when the insert fails.

Copy link
Member

Choose a reason for hiding this comment

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

The let block might be superfluous if we're not handling those errors. We could just inline (sql/insert! tx :friend_requests content)

{:result query-result :error nil})
{:result nil :error (:username-not-exists error-table)})))

(defn accept
[db id]
(jdbc/with-transaction [tx (db)]
(if (exists? tx id)
(let [query-result (sql/update! (db) :friend_requests {:request_state "accepted"} {:id id})
Copy link
Member

Choose a reason for hiding this comment

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

This should use tx

update-count (:next.jdbc/update-count query-result)]
(if (= 1 update-count)
{:result id :error nil}
{:result nil :error (:db-state-error error-table)}))
{:result nil :error (:friend-request-not-exists error-table)})))

(defn reject
Copy link
Member

Choose a reason for hiding this comment

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

Extract the common code out into an update-request-state

[db id]
(jdbc/with-transaction [tx (db)]
(if (exists? tx id)
(let [query-result (sql/update! (db) :friend_requests {:request_state "rejected"} {:id id})
update-count (:next.jdbc/update-count query-result)]
(if (= 1 update-count)
{:result id :error nil}
{:result nil :error (:db-state-error error-table)}))
{:result nil :error (:friend-request-not-exists error-table)})))

(comment
(def db (:db chaat.app/chaat-system))
(chaat.model.user/create db "shahn" "12345678")
(chaat.model.user/create db "neena" "12345678")
(def sample {:sender_username "shahn"
:recipient_username "neena"
:request_state "pending"
:msg "hola"
:creation_timestamp (jt/instant)})
(insert db sample)
(sql/update! (db) :friend_requests {:request_state "accepted"} {:id 0})
(accept db 1)
(reject db 1))
6 changes: 5 additions & 1 deletion src/chaat/handler/errors.clj
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,8 @@
:health-check-error {:msg "Health check error"
:status-code 500}
:unauthorized-action {:msg "Unauthorized action"
:status-code 401}})
:status-code 401}
:friend-request-not-exists {:msg "Friend request does not exist"
:status-code 404}
:db-state-error {:msg "Internal error"
:status-code 500}})
31 changes: 29 additions & 2 deletions src/chaat/handler/handler.clj
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
(ns chaat.handler.handler
(:require [ring.util.response :as res]
[chaat.model.user :as model.user]
[chaat.model.friend-request :as friend-request]
[java-time.api :as jt]
[chaat.handler.validation :as handler.validation]
[chaat.db.utils :as db.utils]
Expand Down Expand Up @@ -61,6 +62,29 @@
(model.user/delete db username))]
(send-response result)))

;; need handler layer validation
(defn create-friend-request
[db {:keys [params] :as request}]
(let [username (:username params)
content (select-keys params [:username :recipient :message])
result (until-err-> (is-auth-user request username)
(friend-request/create db content))]
(send-response result)))

;; need authentication
Copy link
Member

Choose a reason for hiding this comment

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

And authorization – Shahn

(defn accept-friend-request
[db {:keys [route-params]}]
(let [id (Integer/parseInt (:id route-params))
result (friend-request/accept db id)]
(send-response result)))

;; need authentication
(defn reject-friend-request
[db {:keys [route-params]}]
(let [id (Integer/parseInt (:id route-params))
result (friend-request/reject db id)]
(send-response result)))

(defn test-page
"Display the request made to this endpoint for debugging purposes"
[request]
Expand All @@ -73,5 +97,8 @@

(comment
(def db (:db chaat.app/chaat-system))
(is-auth-user {} "shahn")
(delete-user))
(def params {:username "shahn"
:recipient "neena"
:message "hola"})
(def request {:params params})
(create-friend-request db request))
4 changes: 3 additions & 1 deletion src/chaat/migrations.clj
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,6 @@

(comment
{:connection-uri
"jdbc:postgresql://127.0.0.1:8001/chaat_db?user=chaat_dev&password=password"})
"jdbc:postgresql://127.0.0.1:8001/chaat_db?user=chaat_dev&password=password"}
(rollback (config/get-pg-dbspec))
(run-migrations (config/get-pg-dbspec)))
37 changes: 37 additions & 0 deletions src/chaat/model/friend_request.clj
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
(ns chaat.model.friend-request
(:require
[chaat.db.friend-request :as db.friend-request]
[java-time.api :as jt]))


(defn gen-friend-request
[{:keys [username recipient message]}]
{:sender_username username
:recipient_username recipient
:request_state "pending"
:msg message
:creation_timestamp (jt/instant)})

(defn create
[db content]
(let [;; put model layer validation
Copy link
Member

Choose a reason for hiding this comment

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

You may want to keep validations at your boundaries.

friend-request (gen-friend-request content)
result (db.friend-request/insert db friend-request)]
result))

(defn accept
[db id]
(let [;; put model layer validation
result (db.friend-request/accept db id)]
result))

(defn reject
[db id]
(let [;; put model layer validation
result (db.friend-request/reject db id)]
result))

(comment
(def db (:db chaat.app/chaat-system))
(def content {:username "shahn" :recipient "neena" :message "hello"})
(create db content))
3 changes: 2 additions & 1 deletion src/chaat/model/user.clj
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@

(comment
(def db (:db chaat.app/chaat-system))
(create db "shahn" "12345678")
(create db "neena" "12345678")
(authenticate (:result (db.user/get-user-details db "john")) "12345678")
(delete db "john")
(login db "john" "12345678"))
3 changes: 3 additions & 0 deletions src/chaat/routes.clj
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,8 @@
["users" {:post #(handler/signup db %)
:delete (wrap-authentication #(handler/delete-user db %) backend)}]
["login" {:post #(handler/login db %)}]
["friend-requests/" {:post {"" (wrap-authentication #(handler/create-friend-request db %) backend)
[:id "/accept"] (wrap-authentication #(handler/accept-friend-request db %) backend)
[:id "/reject"] (wrap-authentication #(handler/reject-friend-request db %) backend)}}]
["test-page" {:get handler/test-page}]
[true handler/not-found]]])