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

Allow creating a saved annotations with the same title #4884

Merged
merged 7 commits into from
Sep 13, 2023
Merged
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
37 changes: 24 additions & 13 deletions app/assets/javascripts/components/annotations/annotation_form.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,12 @@
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.

Check warning on line 175 in app/assets/javascripts/components/annotations/annotation_form.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/annotations/annotation_form.ts#L173-L175

Added lines #L173 - L175 were not covered by tests
}

const event = new CustomEvent("submit", {
detail: {
text: this._annotationText,
Expand Down Expand Up @@ -229,6 +235,10 @@
])) || []).length > 0;
}

get isTitleTaken(): boolean {
return savedAnnotationState.isTitleTaken(

Check warning on line 239 in app/assets/javascripts/components/annotations/annotation_form.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/annotations/annotation_form.ts#L238-L239

Added lines #L238 - L239 were not covered by tests
this.savedAnnotationTitle, exerciseState.id, courseState.id, userState.id);
}

render(): TemplateResult {
return html`
Expand Down Expand Up @@ -288,19 +298,20 @@
</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()}

Check warning on line 307 in app/assets/javascripts/components/annotations/annotation_form.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/annotations/annotation_form.ts#L306-L307

Added lines #L306 - L307 were not covered by tests
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>
`: ""}

Check warning on line 314 in app/assets/javascripts/components/annotations/annotation_form.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/annotations/annotation_form.ts#L314

Added line #L314 was not covered by tests
</div>
` : ""}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
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
Expand Down Expand Up @@ -47,7 +50,16 @@
};
}

get isTitleTaken(): boolean {
return savedAnnotationState.isTitleTaken(

Check warning on line 54 in app/assets/javascripts/components/saved_annotations/new_saved_annotation.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/new_saved_annotation.ts#L53-L54

Added lines #L53 - L54 were not covered by tests
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;

Check warning on line 60 in app/assets/javascripts/components/saved_annotations/new_saved_annotation.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/new_saved_annotation.ts#L60

Added line #L60 was not covered by tests
}

try {
await savedAnnotationState.create({
from: this.fromAnnotationId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
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
Expand Down Expand Up @@ -46,8 +47,10 @@
<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)}>

Check warning on line 52 in app/assets/javascripts/components/saved_annotations/saved_annotation_form.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/saved_annotation_form.ts#L52

Added line #L52 was not covered by tests
</d-saved-annotation-title-input>
</div>
<div class="field form-group">
<label class="form-label">
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Check warning on line 16 in app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts#L16

Added line #L16 was not covered by tests
@property({ type: Number, attribute: "course-id" })
courseId: number = courseState.id;

Check warning on line 18 in app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts#L18

Added line #L18 was not covered by tests
@property({ type: Number, attribute: "user-id" })
userId: number = userState.id;

Check warning on line 20 in app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts#L20

Added line #L20 was not covered by tests

@property({ state: true })
_value: string;

get isTitleTaken(): boolean {
return savedAnnotationState.isTitleTaken(

Check warning on line 26 in app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts#L25-L26

Added lines #L25 - L26 were not covered by tests
this._value ?? this.value, this.exerciseId, this.courseId, this.userId, this.savedAnnotationId);
}

render(): TemplateResult {
return html`<input required="required"

Check warning on line 31 in app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts#L30-L31

Added lines #L30 - L31 were not covered by tests
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}>

Check warning on line 36 in app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/components/saved_annotations/saved_annotation_title_input.ts#L36

Added line #L36 was not covered by tests
<div class="invalid-feedback">
${I18n.t("js.saved_annotation.title_taken")}
</div>
`;
}
}
4 changes: 4 additions & 0 deletions app/assets/javascripts/i18n/translations.json
Original file line number Diff line number Diff line change
Expand Up @@ -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?"
Expand Down Expand Up @@ -335,6 +336,7 @@
"title": "Saved comments"
},
"title": "Title",
"title_taken": "This title is already taken",
"user": "User"
},
"score": {
Expand Down Expand Up @@ -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?"
Expand Down Expand Up @@ -853,6 +856,7 @@
"title": "Opgeslagen opmerkingen"
},
"title": "Titel",
"title_taken": "Deze titel is al in gebruik",
"user": "Gebruiker"
},
"score": {
Expand Down
11 changes: 11 additions & 0 deletions app/assets/javascripts/state/SavedAnnotations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,17 @@
this.byId.delete(id);
}
}

isTitleTaken(title: string, exerciseId: number, courseId: number, userId: number, savedAnnotationId: number = undefined): boolean {
const params = new Map([

Check warning on line 112 in app/assets/javascripts/state/SavedAnnotations.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/state/SavedAnnotations.ts#L112

Added line #L112 was not covered by tests
["filter", title],
["exercise_id", exerciseId.toString()],
["course_id", courseId.toString()],
["user_id", userId.toString()],
]);
const list = this.getList(params);

Check warning on line 118 in app/assets/javascripts/state/SavedAnnotations.ts

View check run for this annotation

Codecov / codecov/patch

app/assets/javascripts/state/SavedAnnotations.ts#L118

Added line #L118 was not covered by tests
return list?.find(annotation => annotation.title === title && annotation.id != savedAnnotationId) !== undefined;
}
}

export const savedAnnotationState = new SavedAnnotationState();
1 change: 1 addition & 0 deletions app/javascript/packs/saved_annotation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import "components/saved_annotations/saved_annotation_title_input";
2 changes: 1 addition & 1 deletion app/models/saved_annotation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
21 changes: 14 additions & 7 deletions app/views/saved_annotations/edit.html.erb
Original file line number Diff line number Diff line change
@@ -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">
Expand All @@ -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>
Expand Down
2 changes: 2 additions & 0 deletions config/locales/js/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 2 additions & 0 deletions config/locales/js/nl.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
@@ -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
3 changes: 1 addition & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions test/controllers/saved_annotation_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion test/factories/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/users.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down