Merge pull request #4884 from dodona-edu/enhance/allow-duplicate-names

Allow creating a saved annotations with the same title
dodona-edu · Sep 13, 2023 · 5fbbf71 · 5fbbf71
2 parents 6d994f4 + 17ea3d9
commit 5fbbf71
Show file tree

Hide file tree

Showing 18 changed files with 130 additions and 31 deletions.
diff --git a/app/assets/javascripts/components/annotations/annotation_form.ts b/app/assets/javascripts/components/annotations/annotation_form.ts
@@ -169,6 +169,12 @@ export class AnnotationForm extends watchMixin(ShadowlessLitElement) {
                 return; // Something is wrong, abort.
             }
 
+            if (this.saveAnnotation && this.isTitleTaken &&
+                !confirm(I18n.t("js.saved_annotation.confirm_title_taken"))) {
+                this.disabled = false;
+                return; // User cancelled.
+            }
+
             const event = new CustomEvent("submit", {
                 detail: {
                     text: this._annotationText,
@@ -229,6 +235,10 @@ export class AnnotationForm extends watchMixin(ShadowlessLitElement) {
         ])) || []).length > 0;
     }
 
+    get isTitleTaken(): boolean {
+        return savedAnnotationState.isTitleTaken(
+            this.savedAnnotationTitle, exerciseState.id, courseState.id, userState.id);
+    }
 
     render(): TemplateResult {
         return html`
@@ -288,19 +298,20 @@ export class AnnotationForm extends watchMixin(ShadowlessLitElement) {
                                     </label>
                                 </div>
                                 ${this.saveAnnotation ? html`
-                                        <div class="saved-annotation-title">
-                                            <input required="required"
-                                                   class="form-control"
-                                                   type="text"
-                                                   ${ref(this.titleRef)}
-                                                   @keydown="${e => this.handleKeyDown(e)}"
-                                                   @input=${() => this.handleUpdateTitle()}
-                                                   value=${this.savedAnnotationTitle}
-                                                   id="saved-annotation-title"
-                                            >
-                                            <label for="saved-annotation-title">${I18n.t("js.saved_annotation.title")}:</label>
-                                        </div>
-                                    `: ""}
+                                    <div class="saved-annotation-title">
+                                        <input required="required"
+                                               class="form-control ${this.isTitleTaken ? "is-invalid" : ""}"
+                                               type="text"
+                                               ${ref(this.titleRef)}
+                                               @keydown="${e => this.handleKeyDown(e)}"
+                                               @input=${() => this.handleUpdateTitle()}
+                                               value=${this.savedAnnotationTitle}
+                                               id="saved-annotation-title"
+                                               title="${this.isTitleTaken ? I18n.t("js.saved_annotation.title_taken") : ""}"
+                                        >
+                                        <label for="saved-annotation-title">${I18n.t("js.saved_annotation.title")}:</label>
+                                    </div>
+                                `: ""}
                             </div>
                         ` : ""}
                     </div>

diff --git a/app/assets/javascripts/components/saved_annotations/new_saved_annotation.ts b/app/assets/javascripts/components/saved_annotations/new_saved_annotation.ts
@@ -5,6 +5,9 @@ import { SavedAnnotation, savedAnnotationState } from "state/SavedAnnotations";
 import "./saved_annotation_form";
 import { modalMixin } from "components/modal_mixin";
 import { isBetaCourse } from "saved_annotation_beta";
+import { exerciseState } from "state/Exercises";
+import { courseState } from "state/Courses";
+import { userState } from "state/Users";
 
 /**
  * This component represents an creation button for a saved annotation
@@ -47,7 +50,16 @@ export class NewSavedAnnotation extends modalMixin(ShadowlessLitElement) {
         };
     }
 
+    get isTitleTaken(): boolean {
+        return savedAnnotationState.isTitleTaken(
+            this.newSavedAnnotation.title, exerciseState.id, courseState.id, userState.id);
+    }
+
     async createSavedAnnotation(): Promise<void> {
+        if (this.isTitleTaken && !confirm(I18n.t("js.saved_annotation.confirm_title_taken"))) {
+            return;
+        }
+
         try {
             await savedAnnotationState.create({
                 from: this.fromAnnotationId,

diff --git a/app/assets/javascripts/components/saved_annotations/saved_annotation_form.ts b/app/assets/javascripts/components/saved_annotations/saved_annotation_form.ts
@@ -3,6 +3,7 @@ import { html, TemplateResult } from "lit";
 import { ShadowlessLitElement } from "components/meta/shadowless_lit_element";
 import { SavedAnnotation } from "state/SavedAnnotations";
 import { unsafeHTML } from "lit/directives/unsafe-html.js";
+import "components/saved_annotations/saved_annotation_title_input";
 
 /**
  * This component represents a form for creating or editing saved annotations
@@ -46,8 +47,10 @@ export class SavedAnnotationForm extends ShadowlessLitElement {
                     <label class="form-label">
                         ${I18n.t("js.saved_annotation.title")}
                     </label>
-                    <input required="required" class="form-control" type="text"
-                           .value=${this.savedAnnotation.title} @change=${e => this.updateTitle(e)}>
+                    <d-saved-annotation-title-input
+                        .value=${this.savedAnnotation.title}
+                        @change=${e => this.updateTitle(e)}>
+                    </d-saved-annotation-title-input>
                 </div>
                 <div class="field form-group">
                     <label class="form-label">

diff --git a/app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts b/app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts
@@ -0,0 +1,42 @@
+import { customElement, property } from "lit/decorators.js";
+import { html, TemplateResult } from "lit";
+import { ShadowlessLitElement } from "components/meta/shadowless_lit_element";
+import { savedAnnotationState } from "state/SavedAnnotations";
+import { exerciseState } from "state/Exercises";
+import { courseState } from "state/Courses";
+import { userState } from "state/Users";
+
+@customElement("d-saved-annotation-title-input")
+export class SavedAnnotationTitleInput extends ShadowlessLitElement {
+    @property({ type: String })
+    value: string;
+    @property({ type: Number, attribute: "saved-annotation-id" })
+    savedAnnotationId: number;
+    @property({ type: Number, attribute: "exercise-id" })
+    exerciseId: number = exerciseState.id;
+    @property({ type: Number, attribute: "course-id" })
+    courseId: number = courseState.id;
+    @property({ type: Number, attribute: "user-id" })
+    userId: number = userState.id;
+
+    @property({ state: true })
+    _value: string;
+
+    get isTitleTaken(): boolean {
+        return savedAnnotationState.isTitleTaken(
+            this._value ?? this.value, this.exerciseId, this.courseId, this.userId, this.savedAnnotationId);
+    }
+
+    render(): TemplateResult {
+        return html`<input required="required"
+                           class="form-control ${this.isTitleTaken ? "is-invalid" : ""}"
+                           type="text"
+                           name="saved_annotation[title]"
+                           .value=${this.value}
+                           @input=${e => this._value = (e.target as HTMLInputElement).value}>
+            <div class="invalid-feedback">
+                ${I18n.t("js.saved_annotation.title_taken")}
+            </div>
+        `;
+    }
+}
diff --git a/app/assets/javascripts/i18n/translations.json b/app/assets/javascripts/i18n/translations.json
@@ -297,6 +297,7 @@
       "saved_annotation": {
         "annotation_text": "Text",
         "annotations_count": "Number of usages",
+        "confirm_title_taken": "This title is already taken. Are you sure you want to create another annotation with the same title?",
         "course": "Course",
         "delete": {
           "confirm": "Are you sure you want to delete this saved comment?"
@@ -335,6 +336,7 @@
           "title": "Saved comments"
         },
         "title": "Title",
+        "title_taken": "This title is already taken",
         "user": "User"
       },
       "score": {
@@ -815,6 +817,7 @@
       "saved_annotation": {
         "annotation_text": "Tekst",
         "annotations_count": "Aantal keer gebruikt",
+        "confirm_title_taken": "Deze titel is al in gebruik. Weet je zeker dat je nog een opmerking met dezelfde titel wil toevoegen?",
         "course": "Cursus",
         "delete": {
           "confirm": "Ben je zeker dat je deze opgeslagen opmerking wilt verwijderen?"
@@ -853,6 +856,7 @@
           "title": "Opgeslagen opmerkingen"
         },
         "title": "Titel",
+        "title_taken": "Deze titel is al in gebruik",
         "user": "Gebruiker"
       },
       "score": {

diff --git a/app/assets/javascripts/state/SavedAnnotations.ts b/app/assets/javascripts/state/SavedAnnotations.ts
@@ -107,6 +107,17 @@ class SavedAnnotationState extends State {
             this.byId.delete(id);
         }
     }
+
+    isTitleTaken(title: string, exerciseId: number, courseId: number, userId: number, savedAnnotationId: number = undefined): boolean {
+        const params = new Map([
+            ["filter", title],
+            ["exercise_id", exerciseId.toString()],
+            ["course_id", courseId.toString()],
+            ["user_id", userId.toString()],
+        ]);
+        const list = this.getList(params);
+        return list?.find(annotation => annotation.title === title && annotation.id != savedAnnotationId) !== undefined;
+    }
 }
 
 export const savedAnnotationState = new SavedAnnotationState();
diff --git a/app/javascript/packs/saved_annotation.js b/app/javascript/packs/saved_annotation.js
@@ -0,0 +1 @@
+import "components/saved_annotations/saved_annotation_title_input";
diff --git a/app/models/saved_annotation.rb b/app/models/saved_annotation.rb
@@ -13,7 +13,7 @@
 #  annotations_count :integer          default(0)
 #
 class SavedAnnotation < ApplicationRecord
-  validates :title, presence: true, uniqueness: { scope: %i[user_id exercise_id course_id] }
+  validates :title, presence: true
   validates :annotation_text, presence: true
 
   belongs_to :user

diff --git a/app/models/user.rb b/app/models/user.rb
@@ -18,7 +18,7 @@
 #  seen_at              :datetime
 #  sign_in_at           :datetime
 #  open_questions_count :integer          default(0), not null
-#  theme                :integer          default(0), not null
+#  theme                :integer          default("system"), not null
 #
 
 require 'securerandom'

diff --git a/app/views/saved_annotations/edit.html.erb b/app/views/saved_annotations/edit.html.erb
@@ -1,3 +1,4 @@
+<%= javascript_include_tag "saved_annotation" %>
 <div class="row">
   <div class="col-md-10 offset-md-1 col-12">
     <div class="card">
@@ -17,14 +18,20 @@
               </ul>
             </div>
           <% end %>
-          <div class="field form-group row">
-            <%= f.label :title, class: "col-sm-3 col-form-label" %>
-            <div class="col-sm-6"><%= f.text_field :title, class: "form-control" %></div>
+          <div class="field form-group">
+            <%= f.label :title, class: "form-label" %>
+            <d-saved-annotation-title-input
+              value="<%= @saved_annotation.title %>"
+              course-id="<%= @saved_annotation.course_id %>"
+              exercise-id="<%= @saved_annotation.exercise_id %>"
+              user-id="<%= @saved_annotation.user_id %>"
+              saved-annotation-id="<%= @saved_annotation.id %>"
+            ></d-saved-annotation-title-input>
           </div>
-          <div class="field form-group row">
-            <%= f.label :annotation_text, class: "col-sm-3 col-form-label" %>
-            <div class="col-sm-6"><%= f.text_area :annotation_text, class: "form-control" %></div>
-            <span class="help-block offset-sm-3 col-sm-6"><%= t ".markdown_html" %></span>
+          <div class="field form-group">
+            <%= f.label :annotation_text, class: "form-label" %>
+            <%= f.text_area :annotation_text, class: "form-control" %>
+            <span class="help-block"><%= t ".markdown_html" %></span>
           </div>
         <% end %>
       </div>

diff --git a/config/locales/js/en.yml b/config/locales/js/en.yml
@@ -264,6 +264,8 @@ en:
       course: Course
       exercise: Exercise
       annotations_count: Number of usages
+      title_taken: 'This title is already taken'
+      confirm_title_taken: 'This title is already taken. Are you sure you want to create another annotation with the same title?'
       list:
         annotations_count: "Used %{count} times"
       sidecard:

diff --git a/config/locales/js/nl.yml b/config/locales/js/nl.yml
@@ -264,6 +264,8 @@ nl:
       course: Cursus
       exercise: Oefening
       annotations_count: "Aantal keer gebruikt"
+      title_taken: 'Deze titel is al in gebruik'
+      confirm_title_taken: 'Deze titel is al in gebruik. Weet je zeker dat je nog een opmerking met dezelfde titel wil toevoegen?'
       list:
         annotations_count: "%{count} keer gebruikt"
       sidecard:

diff --git a/db/migrate/20230810105908_remove_unique_key_on_saved_annotation_title.rb b/db/migrate/20230810105908_remove_unique_key_on_saved_annotation_title.rb
@@ -0,0 +1,5 @@
+class RemoveUniqueKeyOnSavedAnnotationTitle < ActiveRecord::Migration[7.0]
+  def change
+    remove_index :saved_annotations,  name: "index_saved_annotations_title_uid_eid_cid"
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_07_13_073547) do
+ActiveRecord::Schema[7.0].define(version: 2023_08_10_105908) do
   create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
     t.string "name", null: false
     t.string "record_type", null: false
@@ -424,7 +424,6 @@
     t.integer "annotations_count", default: 0
     t.index ["course_id"], name: "index_saved_annotations_on_course_id"
     t.index ["exercise_id"], name: "index_saved_annotations_on_exercise_id"
-    t.index ["title", "user_id", "exercise_id", "course_id"], name: "index_saved_annotations_title_uid_eid_cid", unique: true
     t.index ["user_id"], name: "index_saved_annotations_on_user_id"
   end
 

diff --git a/test/controllers/saved_annotation_controller_test.rb b/test/controllers/saved_annotation_controller_test.rb
@@ -22,13 +22,13 @@ def setup
     assert_response :success
   end
 
-  test 'creating a saved annotation should fail when one with the same name already exists' do
+  test 'creating a saved annotation should work when one with the same name already exists' do
     annotation = create :annotation, submission:  create(:submission, course: @course), user: @user
     post saved_annotations_url, params: { format: :json, saved_annotation: { title: 'test', annotation_text: annotation.annotation_text }, from: annotation.id }
 
     assert_response :success
     post saved_annotations_url, params: { format: :json, saved_annotation: { title: 'test', annotation_text: annotation.annotation_text }, from: annotation.id }
 
-    assert_response :unprocessable_entity
+    assert_response :success
   end
 end
diff --git a/test/factories/users.rb b/test/factories/users.rb
@@ -18,7 +18,7 @@
 #  seen_at              :datetime
 #  sign_in_at           :datetime
 #  open_questions_count :integer          default(0), not null
-#  theme                :integer          default(0), not null
+#  theme                :integer          default("system"), not null
 #
 
 FactoryBot.define do

diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml
@@ -18,7 +18,7 @@
 #  seen_at              :datetime
 #  sign_in_at           :datetime
 #  open_questions_count :integer          default(0), not null
-#  theme                :integer          default(0), not null
+#  theme                :integer          default("system"), not null
 #
 
 zeus:

diff --git a/test/models/user_test.rb b/test/models/user_test.rb
@@ -18,7 +18,7 @@
 #  seen_at              :datetime
 #  sign_in_at           :datetime
 #  open_questions_count :integer          default(0), not null
-#  theme                :integer          default(0), not null
+#  theme                :integer          default("system"), not null
 #
 
 require 'test_helper'