Merge #292: Do not allow empty category names

c2b7488 feat!: [#288] do not allow empty category names (Jose Celano)

Pull request description:

  It was allowed to use an empty string like "" or " " for a category name.

  From now on, it's not allowed.

  If there were some empty names in the database, they were not renamed. Admins must optionally do that. Names were anyway UNIQUE.

  A migration to rename empty names was not added because there can be more than one category, for example:

  - ""
  - " "
  - "  "
  - Etcetera

  We could have generated names like "no category 1", "no category 2", but it's not likely that admins have created empty categories.

ACKs for top commit:
  josecelano:
    ACK c2b7488

Tree-SHA512: 72a2b41d3757b6bf54178448f14644e0449765b3afede9c1e4a080270ac91ba00742072431091a767322ffa5b4f89f4df42a2ae4b77523005b300319bc7e71d7

src/errors.rs

@@ -130,6 +130,9 @@ pub enum ServiceError {
     #[display(fmt = "Category already exists.")]
     CategoryAlreadyExists,
 
+    #[display(fmt = "Category name cannot be empty.")]
+    CategoryNameEmpty,
+
     #[display(fmt = "Tag already exists.")]
     TagAlreadyExists,
 
@@ -235,6 +238,7 @@ pub fn http_status_code_for_service_error(error: &ServiceError) -> StatusCode {
         ServiceError::CanonicalInfoHashAlreadyExists => StatusCode::BAD_REQUEST,
         ServiceError::TorrentTitleAlreadyExists => StatusCode::BAD_REQUEST,
         ServiceError::TrackerOffline => StatusCode::INTERNAL_SERVER_ERROR,
+        ServiceError::CategoryNameEmpty => StatusCode::BAD_REQUEST,
         ServiceError::CategoryAlreadyExists => StatusCode::BAD_REQUEST,
         ServiceError::TagAlreadyExists => StatusCode::BAD_REQUEST,
         ServiceError::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR,

src/services/category.rs

@@ -38,7 +38,13 @@ impl Service {
             return Err(ServiceError::Unauthorized);
         }
 
-        match self.category_repository.add(category_name).await {
+        let trimmed_name = category_name.trim();
+
+        if trimmed_name.is_empty() {
+            return Err(ServiceError::CategoryNameEmpty);
+        }
+
+        match self.category_repository.add(trimmed_name).await {
             Ok(id) => Ok(id),
             Err(e) => match e {
                 DatabaseError::CategoryAlreadyExists => Err(ServiceError::CategoryAlreadyExists),

tests/e2e/web/api/v1/contexts/category/contract.rs

@@ -104,32 +104,25 @@ async fn it_should_allow_admins_to_add_new_categories() {
 }
 
 #[tokio::test]
-async fn it_should_allow_adding_empty_categories() {
-    // code-review: this is a bit weird, is it a intended behavior?
-
+async fn it_should_not_allow_adding_empty_categories() {
     let mut env = TestEnv::new();
     env.start(api::Version::V1).await;
 
-    if env.is_shared() {
-        // This test cannot be run in a shared test env because it will fail
-        // when the empty category already exits
-        println!("Skipped");
-        return;
-    }
-
     let logged_in_admin = new_logged_in_admin(&env).await;
     let client = Client::authenticated(&env.server_socket_addr().unwrap(), &logged_in_admin.token);
 
-    let category_name = String::new();
+    let invalid_category_names = vec![String::new(), " ".to_string()];
 
-    let response = client
-        .add_category(AddCategoryForm {
-            name: category_name.to_string(),
-            icon: None,
-        })
-        .await;
+    for invalid_name in invalid_category_names {
+        let response = client
+            .add_category(AddCategoryForm {
+                name: invalid_name,
+                icon: None,
+            })
+            .await;
 
-    assert_added_category_response(&response, &category_name);
+        assert_eq!(response.status, 400);
+    }
 }
 
 #[tokio::test]