feat: ability to delete a project #37

Merged
matous-volf merged 4 commits from feat/project-delete into main 2024-09-08 06:39:56 +00:00
6 changed files with 51 additions and 16 deletions

View File

@ -6,6 +6,6 @@ CREATE TABLE "tasks"(
"deadline" DATE, "deadline" DATE,
"category" JSONB NOT NULL, "category" JSONB NOT NULL,
"project_id" INT4, "project_id" INT4,
FOREIGN KEY ("project_id") REFERENCES "projects"("id") FOREIGN KEY ("project_id") REFERENCES "projects"("id") ON DELETE SET NULL
); );

View File

@ -1,10 +1,10 @@
use crate::models::project::{NewProject, Project}; use crate::models::project::{NewProject, Project};
use crate::server::projects::{create_project, edit_project}; use crate::query::{QueryErrors, QueryKey, QueryValue};
use crate::server::projects::{create_project, delete_project, edit_project};
use dioxus::core_macro::{component, rsx}; use dioxus::core_macro::{component, rsx};
use dioxus::dioxus_core::Element; use dioxus::dioxus_core::Element;
use dioxus::prelude::*; use dioxus::prelude::*;
use dioxus_query::prelude::use_query_client; use dioxus_query::prelude::use_query_client;
use crate::query::{QueryErrors, QueryKey, QueryValue};
#[component] #[component]
pub(crate) fn ProjectForm(project: Option<Project>, on_successful_submit: EventHandler<()>) pub(crate) fn ProjectForm(project: Option<Project>, on_successful_submit: EventHandler<()>)
@ -25,9 +25,7 @@ pub(crate) fn ProjectForm(project: Option<Project>, on_successful_submit: EventH
} else { } else {
let _ = create_project(new_project).await; let _ = create_project(new_project).await;
} }
query_client.invalidate_queries(&[ query_client.invalidate_queries(&[QueryKey::Projects]);
QueryKey::Projects
]);
on_successful_submit.call(()); on_successful_submit.call(());
} }
}, },
@ -44,14 +42,31 @@ pub(crate) fn ProjectForm(project: Option<Project>, on_successful_submit: EventH
input { input {
name: "title", name: "title",
required: true, required: true,
initial_value: project.map(|project| project.title().to_owned()), initial_value: project.as_ref().map(|project| project.title().to_owned()),
r#type: "text", r#type: "text",
class: "py-2 px-3 grow bg-zinc-800/50 rounded-lg", class: "py-2 px-3 grow bg-zinc-800/50 rounded-lg",
id: "input_title" id: "input_title"
} }
} }
div { div {
class: "flex flex-row justify-end mt-auto", class: "flex flex-row justify-between mt-auto",
button {
r#type: "button",
class: "py-2 px-4 bg-zinc-300/50 rounded-lg",
onclick: move |_| {
let project = project.clone();
async move {
if let Some(project) = project {
let _ = delete_project(project.id()).await;
query_client.invalidate_queries(&[QueryKey::Projects]);
}
on_successful_submit.call(());
}
},
i {
class: "fa-solid fa-trash-can"
}
}
button { button {
r#type: "submit", r#type: "submit",
class: "py-2 px-4 bg-zinc-300/50 rounded-lg", class: "py-2 px-4 bg-zinc-300/50 rounded-lg",

View File

@ -7,11 +7,17 @@ pub enum Error {
ServerInternal, ServerInternal,
} }
impl From<diesel::result::Error> for Error {
fn from(_: diesel::result::Error) -> Self {
Self::ServerInternal
}
coderabbitai[bot] commented 2024-09-08 06:25:55 +00:00 (Migrated from github.com)
Review

Refine error mapping in From implementation.

The current implementation maps all diesel::result::Error types to Error::ServerInternal. Consider refining this to differentiate between different types of database errors, such as constraint violations or connection issues, to provide more specific error messages to the client.

**Refine error mapping in `From` implementation.** The current implementation maps all `diesel::result::Error` types to `Error::ServerInternal`. Consider refining this to differentiate between different types of database errors, such as constraint violations or connection issues, to provide more specific error messages to the client. <!-- This is an auto-generated comment by CodeRabbit -->
}
// has to be implemented for Dioxus server functions // has to be implemented for Dioxus server functions
impl Display for Error { impl Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self { match self {
Error::ServerInternal => write!(f, "internal server error"), Self::ServerInternal => write!(f, "internal server error"),
} }
} }
} }
@ -22,7 +28,7 @@ impl FromStr for Error {
fn from_str(s: &str) -> Result<Self, Self::Err> { fn from_str(s: &str) -> Result<Self, Self::Err> {
Ok(match s { Ok(match s {
"internal server error" => Error::ServerInternal, "internal server error" => Self::ServerInternal,
_ => return Err(()), _ => return Err(()),
}) })
} }

View File

@ -36,7 +36,7 @@ impl From<ValidationErrors> for ErrorVec<ProjectError> {
impl From<diesel::result::Error> for ProjectError { impl From<diesel::result::Error> for ProjectError {
fn from(_: diesel::result::Error) -> Self { fn from(_: diesel::result::Error) -> Self {
ProjectError::Error(Error::ServerInternal) Self::Error(Error::ServerInternal)
} }
} }
@ -52,6 +52,6 @@ impl FromStr for ProjectError {
type Err = (); type Err = ();
fn from_str(_: &str) -> Result<Self, Self::Err> { fn from_str(_: &str) -> Result<Self, Self::Err> {
Ok(ProjectError::Error(Error::ServerInternal)) Ok(Self::Error(Error::ServerInternal))
} }
} }

View File

@ -42,12 +42,12 @@ impl From<diesel::result::Error> for TaskError {
diesel::result::DatabaseErrorKind::ForeignKeyViolation, info diesel::result::DatabaseErrorKind::ForeignKeyViolation, info
) => { ) => {
match info.constraint_name() { match info.constraint_name() {
Some("tasks_project_id_fkey") => TaskError::ProjectNotFound, Some("tasks_project_id_fkey") => Self::ProjectNotFound,
_ => TaskError::Error(Error::ServerInternal) _ => Self::Error(Error::ServerInternal)
} }
} }
_ => { _ => {
TaskError::Error(Error::ServerInternal) Self::Error(Error::ServerInternal)
} }
} }
} }
@ -65,6 +65,6 @@ impl FromStr for TaskError {
type Err = (); type Err = ();
fn from_str(_: &str) -> Result<Self, Self::Err> { fn from_str(_: &str) -> Result<Self, Self::Err> {
Ok(TaskError::Error(Error::ServerInternal)) Ok(Self::Error(Error::ServerInternal))
} }
} }

View File

@ -71,3 +71,17 @@ pub(crate) async fn edit_project(project_id: i32, new_project: NewProject)
Ok(updated_project) Ok(updated_project)
} }
#[server]
pub(crate) async fn delete_project(project_id: i32)
-> Result<(), ServerFnError<ErrorVec<Error>>> {
use crate::schema::projects::dsl::*;
let mut connection = establish_database_connection()
.map_err::<ErrorVec<Error>, _>(|_| vec![Error::ServerInternal].into())?;
diesel::delete(projects.filter(id.eq(project_id))).execute(&mut connection)
.map_err::<ErrorVec<Error>, _>(|error| vec![error.into()].into())?;
Ok(())
}
coderabbitai[bot] commented 2024-09-08 06:25:55 +00:00 (Migrated from github.com)
Review

Approve the implementation of delete_project with a suggestion for enhancement.

The implementation of delete_project is correct and aligns well with the existing codebase. The error handling is consistent and uses the application's custom error types effectively.

However, consider enhancing the database operation by wrapping it in a transaction. This ensures that any related operations that need to be atomic with the deletion are handled correctly.

Consider adding transaction handling to ensure atomicity:

let mut connection = establish_database_connection()
    .map_err::<ErrorVec<Error>, _>(|_| vec![Error::ServerInternal].into())?;

connection.transaction::<_, Error, _>(|| {
    diesel::delete(projects.filter(id.eq(project_id))).execute(&mut connection)
        .map_err::<ErrorVec<Error>, _>(|error| vec![error.into()].into())
});
**Approve the implementation of `delete_project` with a suggestion for enhancement.** The implementation of `delete_project` is correct and aligns well with the existing codebase. The error handling is consistent and uses the application's custom error types effectively. However, consider enhancing the database operation by wrapping it in a transaction. This ensures that any related operations that need to be atomic with the deletion are handled correctly. Consider adding transaction handling to ensure atomicity: ```rust let mut connection = establish_database_connection() .map_err::<ErrorVec<Error>, _>(|_| vec![Error::ServerInternal].into())?; connection.transaction::<_, Error, _>(|| { diesel::delete(projects.filter(id.eq(project_id))).execute(&mut connection) .map_err::<ErrorVec<Error>, _>(|error| vec![error.into()].into()) }); ``` <!-- This is an auto-generated reply by CodeRabbit -->
matous-volf commented 2024-09-08 06:36:13 +00:00 (Migrated from github.com)
Review

What's the point of a transaction when only one query is being executed?

What's the point of a transaction when only one query is being executed?
coderabbitai[bot] commented 2024-09-08 06:36:42 +00:00 (Migrated from github.com)
Review

@matous-volf, you're right that a transaction is typically more useful when multiple queries are involved. In this case, since only one query is being executed, a transaction might seem unnecessary.

However, there are a few reasons why it could still be beneficial:

  1. Consistency: Even with a single query, a transaction ensures that the operation is either fully completed or fully rolled back in case of unexpected issues (e.g., database crashes or connection problems).
  2. Future-proofing: If additional operations (like cascading deletes or logging) are added later, the transaction ensures that all related operations are atomic.
  3. Locking behavior: In some databases, transactions can help manage locking more effectively, even for single queries.

That said, if you're confident that this single query is sufficient and no future operations will be added, the transaction may not be necessary in this specific case.

@matous-volf, you're right that a transaction is typically more useful when multiple queries are involved. In this case, since only one query is being executed, a transaction might seem unnecessary. However, there are a few reasons why it could still be beneficial: 1. **Consistency**: Even with a single query, a transaction ensures that the operation is either fully completed or fully rolled back in case of unexpected issues (e.g., database crashes or connection problems). 2. **Future-proofing**: If additional operations (like cascading deletes or logging) are added later, the transaction ensures that all related operations are atomic. 3. **Locking behavior**: In some databases, transactions can help manage locking more effectively, even for single queries. That said, if you're confident that this single query is sufficient and no future operations will be added, the transaction may not be necessary in this specific case. <!-- This is an auto-generated reply by CodeRabbit -->