From f517b6997cacbf9d5602ad07da5642b4af20b7fa Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Tue, 14 Sep 2021 15:18:01 +0300 Subject: [PATCH] FEATURE: Cook drafts excerpt in user activity (#14315) The previous excerpt was a simple truncated raw message. Starting with this commit, the raw content of the draft is cooked and an excerpt is extracted from it. The logic for extracting the excerpt mimics the the `ExcerptParser` class, but does not implement all functionality, being a much simpler implementation. The two draft controllers have been merged into one and the /draft.json route has been changed to /drafts.json to be consistent with the other route names. --- .../javascripts/discourse/app/lib/text.js | 50 ++++ .../javascripts/discourse/app/models/draft.js | 9 +- .../app/models/user-drafts-stream.js | 111 ++++---- .../app/routes/user-activity-drafts.js | 5 +- .../acceptance/composer-draft-saving-test.js | 2 +- .../acceptance/composer-edit-conflict-test.js | 2 +- .../acceptance/user-drafts-stream-test.js | 14 + .../discourse/tests/fixtures/draft.js | 21 -- .../discourse/tests/fixtures/drafts.js | 17 +- .../tests/helpers/create-pretender.js | 22 +- .../tests/unit/models/user-drafts-test.js | 12 +- app/controllers/draft_controller.rb | 86 ------- app/controllers/drafts_controller.rb | 93 ++++++- app/models/draft.rb | 6 +- config/routes.rb | 5 +- spec/requests/draft_controller_spec.rb | 175 ------------- spec/requests/drafts_controller_spec.rb | 243 ++++++++++++++---- spec/requests/session_controller_spec.rb | 2 +- 18 files changed, 438 insertions(+), 437 deletions(-) delete mode 100644 app/assets/javascripts/discourse/tests/fixtures/draft.js delete mode 100644 app/controllers/draft_controller.rb delete mode 100644 spec/requests/draft_controller_spec.rb diff --git a/app/assets/javascripts/discourse/app/lib/text.js b/app/assets/javascripts/discourse/app/lib/text.js index f83d5069131..ea40982c6c3 100644 --- a/app/assets/javascripts/discourse/app/lib/text.js +++ b/app/assets/javascripts/discourse/app/lib/text.js @@ -110,3 +110,53 @@ export function emojiUrlFor(code) { return buildEmojiUrl(code, opts); } } + +function encode(str) { + return str.replaceAll("<", "<").replaceAll(">", ">"); +} + +function traverse(element, callback) { + if (callback(element)) { + element.childNodes.forEach((child) => traverse(child, callback)); + } +} + +export function excerpt(cooked, length) { + let result = ""; + let resultLength = 0; + + const div = document.createElement("div"); + div.innerHTML = cooked; + traverse(div, (element) => { + if (resultLength >= length) { + return; + } + + if (element.nodeType === Node.TEXT_NODE) { + if (resultLength + element.textContent.length > length) { + const text = element.textContent.substr(0, length - resultLength); + result += encode(text); + result += "…"; + resultLength += text.length; + } else { + result += encode(element.textContent); + resultLength += element.textContent.length; + } + } else if (element.tagName === "A") { + element.innerHTML = element.innerText; + result += element.outerHTML; + resultLength += element.innerText.length; + } else if (element.tagName === "IMG") { + if (element.classList.contains("emoji")) { + result += element.outerHTML; + } else { + result += "[image]"; + resultLength += "[image]".length; + } + } else { + return true; + } + }); + + return result; +} diff --git a/app/assets/javascripts/discourse/app/models/draft.js b/app/assets/javascripts/discourse/app/models/draft.js index 37dda0ba765..4275cab1630 100644 --- a/app/assets/javascripts/discourse/app/models/draft.js +++ b/app/assets/javascripts/discourse/app/models/draft.js @@ -5,17 +5,14 @@ const Draft = EmberObject.extend(); Draft.reopenClass({ clear(key, sequence) { - return ajax("/draft.json", { + return ajax(`/drafts/${key}.json`, { type: "DELETE", data: { draft_key: key, sequence }, }); }, get(key) { - return ajax("/draft.json", { - data: { draft_key: key }, - dataType: "json", - }); + return ajax(`/drafts/${key}.json`); }, getLocal(key, current) { @@ -25,7 +22,7 @@ Draft.reopenClass({ save(key, sequence, data, clientId, { forceSave = false } = {}) { data = typeof data === "string" ? data : JSON.stringify(data); - return ajax("/draft.json", { + return ajax("/drafts.json", { type: "POST", data: { draft_key: key, diff --git a/app/assets/javascripts/discourse/app/models/user-drafts-stream.js b/app/assets/javascripts/discourse/app/models/user-drafts-stream.js index e703e5acb8a..00d6f04c7c6 100644 --- a/app/assets/javascripts/discourse/app/models/user-drafts-stream.js +++ b/app/assets/javascripts/discourse/app/models/user-drafts-stream.js @@ -1,105 +1,100 @@ +import discourseComputed from "discourse-common/utils/decorators"; +import { ajax } from "discourse/lib/ajax"; +import { cookAsync, emojiUnescape, excerpt } from "discourse/lib/text"; +import { escapeExpression } from "discourse/lib/utilities"; import { NEW_PRIVATE_MESSAGE_KEY, NEW_TOPIC_KEY, } from "discourse/models/composer"; -import { A } from "@ember/array"; -import { Promise } from "rsvp"; import RestModel from "discourse/models/rest"; import UserDraft from "discourse/models/user-draft"; -import { ajax } from "discourse/lib/ajax"; -import discourseComputed from "discourse-common/utils/decorators"; -import { emojiUnescape } from "discourse/lib/text"; -import { escapeExpression } from "discourse/lib/utilities"; -import { url } from "discourse/lib/computed"; +import { Promise } from "rsvp"; export default RestModel.extend({ - loaded: false, + limit: 30, + + loading: false, + hasMore: false, + content: null, init() { this._super(...arguments); + this.reset(); + }, + + reset() { this.setProperties({ - itemsLoaded: 0, + loading: false, + hasMore: true, content: [], - lastLoadedUrl: null, }); }, - baseUrl: url( - "itemsLoaded", - "user.username_lower", - "/drafts.json?offset=%@&username=%@" - ), - - load(site) { - this.setProperties({ - itemsLoaded: 0, - content: [], - lastLoadedUrl: null, - site: site, - }); - return this.findItems(); - }, - - @discourseComputed("content.length", "loaded") - noContent(contentLength, loaded) { - return loaded && contentLength === 0; + @discourseComputed("content.length", "loading") + noContent(contentLength, loading) { + return contentLength === 0 && !loading; }, remove(draft) { - let content = this.content.filter( - (item) => item.draft_key !== draft.draft_key + this.set( + "content", + this.content.filter((item) => item.draft_key !== draft.draft_key) ); - this.setProperties({ content, itemsLoaded: content.length }); }, - findItems() { - let findUrl = this.baseUrl; - - const lastLoadedUrl = this.lastLoadedUrl; - if (lastLoadedUrl === findUrl) { - return Promise.resolve(); + findItems(site) { + if (site) { + this.set("site", site); } - if (this.loading) { + if (this.loading || !this.hasMore) { return Promise.resolve(); } this.set("loading", true); - return ajax(findUrl) + const url = `/drafts.json?offset=${this.content.length}&limit=${this.limit}`; + return ajax(url) .then((result) => { - if (result && result.no_results_help) { + if (!result) { + return; + } + + if (result.no_results_help) { this.set("noContentHelp", result.no_results_help); } - if (result && result.drafts) { - const copy = A(); - result.drafts.forEach((draft) => { - let draftData = JSON.parse(draft.data); - draft.post_number = draftData.postId || null; + + if (!result.drafts) { + return; + } + + this.set("hasMore", result.drafts.size >= this.limit); + + const promises = result.drafts.map((draft) => { + draft.data = JSON.parse(draft.data); + return cookAsync(draft.data.reply).then((cooked) => { + draft.excerpt = excerpt(cooked.string, 300); + draft.post_number = draft.data.postId || null; if ( draft.draft_key === NEW_PRIVATE_MESSAGE_KEY || draft.draft_key === NEW_TOPIC_KEY ) { - draft.title = draftData.title; + draft.title = draft.data.title; } draft.title = emojiUnescape(escapeExpression(draft.title)); - if (draft.category_id) { + if (draft.data.categoryId) { draft.category = - this.site.categories.findBy("id", draft.category_id) || null; + this.site.categories.findBy("id", draft.data.categoryId) || + null; } + this.content.push(UserDraft.create(draft)); + }); + }); - copy.pushObject(UserDraft.create(draft)); - }); - this.content.pushObjects(copy); - this.setProperties({ - loaded: true, - itemsLoaded: this.itemsLoaded + result.drafts.length, - }); - } + return Promise.all(promises); }) .finally(() => { this.set("loading", false); - this.set("lastLoadedUrl", findUrl); }); }, }); diff --git a/app/assets/javascripts/discourse/app/routes/user-activity-drafts.js b/app/assets/javascripts/discourse/app/routes/user-activity-drafts.js index 840015b7b92..1116dee350d 100644 --- a/app/assets/javascripts/discourse/app/routes/user-activity-drafts.js +++ b/app/assets/javascripts/discourse/app/routes/user-activity-drafts.js @@ -2,8 +2,9 @@ import DiscourseRoute from "discourse/routes/discourse"; export default DiscourseRoute.extend({ model() { - let userDraftsStream = this.modelFor("user").get("userDraftsStream"); - return userDraftsStream.load(this.site).then(() => userDraftsStream); + const model = this.modelFor("user").get("userDraftsStream"); + model.reset(); + return model.findItems(this.site).then(() => model); }, renderTemplate() { diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-draft-saving-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-draft-saving-test.js index 44f6b4d87f9..22676e94145 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-draft-saving-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-draft-saving-test.js @@ -13,7 +13,7 @@ acceptance("Composer - Draft saving", function (needs) { const draftThatWillBeSaved = "This_will_be_saved_successfully"; needs.pretender((server, helper) => { - server.post("/draft.json", (request) => { + server.post("/drafts.json", (request) => { const success = request.requestBody.includes(draftThatWillBeSaved); return success ? helper.response({ success: true }) diff --git a/app/assets/javascripts/discourse/tests/acceptance/composer-edit-conflict-test.js b/app/assets/javascripts/discourse/tests/acceptance/composer-edit-conflict-test.js index 96f970237a0..841545e9e4e 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/composer-edit-conflict-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/composer-edit-conflict-test.js @@ -8,7 +8,7 @@ acceptance("Composer - Edit conflict", function (needs) { let lastBody; needs.pretender((server, helper) => { - server.post("/draft.json", (request) => { + server.post("/drafts.json", (request) => { lastBody = request.requestBody; return helper.response({ success: true }); }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-drafts-stream-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-drafts-stream-test.js index 1b2aa090f5b..c762e7ddce5 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-drafts-stream-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/user-drafts-stream-test.js @@ -2,6 +2,7 @@ import { acceptance, count, exists, + query, queryAll, visible, } from "discourse/tests/helpers/qunit-helpers"; @@ -36,4 +37,17 @@ acceptance("User Drafts", function (needs) { "A fun new topic for testing drafts." ); }); + + test("Stream - has excerpt", async function (assert) { + await visit("/u/eviltrout/activity/drafts"); + assert.ok(exists(".user-stream-item"), "has drafts"); + assert.equal( + query(".user-stream-item:nth-child(3) .category").textContent, + "meta" + ); + assert.equal( + query(".user-stream-item:nth-child(3) .excerpt").innerHTML.trim(), + 'here goes a reply to a PM :slight_smile:' + ); + }); }); diff --git a/app/assets/javascripts/discourse/tests/fixtures/draft.js b/app/assets/javascripts/discourse/tests/fixtures/draft.js deleted file mode 100644 index a1d5ce6b9cc..00000000000 --- a/app/assets/javascripts/discourse/tests/fixtures/draft.js +++ /dev/null @@ -1,21 +0,0 @@ -export default { - "/draft.json": { - draft: null, - draft_sequence: 0 - }, - "/draft.json?draft_key=topic_280": { - draft: - '{"reply":"This is a draft of the first post","action":"reply","categoryId":1,"archetypeId":"regular","metaData":null,"composerTime":2863,"typingTime":200}', - draft_sequence: 42 - }, - "/draft.json?draft_key=topic_281": { - draft: - '{"reply":"dum de dum da ba.","action":"createTopic","title":"dum da ba dum dum","categoryId":null,"archetypeId":"regular","metaData":null,"composerTime":540879,"typingTime":3400}', - draft_sequence: 0 - }, - "/draft.json?draft_key=topic_9": { - draft: - '{"reply":"This is a draft of the first post","action":"reply","categoryId":1,"archetypeId":"regular","metaData":null,"composerTime":2863,"typingTime":200}', - draft_sequence: 42 - } -}; diff --git a/app/assets/javascripts/discourse/tests/fixtures/drafts.js b/app/assets/javascripts/discourse/tests/fixtures/drafts.js index f5f31958f6e..31f33ace696 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/drafts.js +++ b/app/assets/javascripts/discourse/tests/fixtures/drafts.js @@ -47,7 +47,7 @@ export default { draft_username: "eviltrout", avatar_template: "/user_avatar/localhost/eviltrout/{size}/2_1.png", data: - '{"reply":"here goes a reply to a PM.","action":"reply","categoryId":null,"postId":212,"archetypeId":"regular","whisper":false,"metaData":null,"composerTime":455711,"typingTime":5400}', + '{"reply":"here goes a reply to a PM :slight_smile:","action":"reply","categoryId":3,"postId":212,"archetypeId":"regular","whisper":false,"metaData":null,"composerTime":455711,"typingTime":5400}', topic_id: 93, username: "eviltrout", name: null, @@ -57,5 +57,20 @@ export default { archetype: "private_message" } ] + }, + "/drafts/topic_280.json": { + draft: + '{"reply":"This is a draft of the first post","action":"reply","categoryId":1,"archetypeId":"regular","metaData":null,"composerTime":2863,"typingTime":200}', + draft_sequence: 42 + }, + "/drafts/topic_281.json": { + draft: + '{"reply":"dum de dum da ba.","action":"createTopic","title":"dum da ba dum dum","categoryId":null,"archetypeId":"regular","metaData":null,"composerTime":540879,"typingTime":3400}', + draft_sequence: 0 + }, + "/drafts/topic_9.json": { + draft: + '{"reply":"This is a draft of the first post","action":"reply","categoryId":1,"archetypeId":"regular","metaData":null,"composerTime":2863,"typingTime":200}', + draft_sequence: 42 } }; diff --git a/app/assets/javascripts/discourse/tests/helpers/create-pretender.js b/app/assets/javascripts/discourse/tests/helpers/create-pretender.js index 96ae6e91b5c..fd1df4131b8 100644 --- a/app/assets/javascripts/discourse/tests/helpers/create-pretender.js +++ b/app/assets/javascripts/discourse/tests/helpers/create-pretender.js @@ -291,8 +291,8 @@ export function applyDefaultHandlers(pretender) { pretender.get("/permalink-check.json", () => response({ found: false })); - pretender.delete("/draft.json", success); - pretender.post("/draft.json", success); + pretender.delete("/drafts/:draft_key.json", success); + pretender.post("/drafts.json", success); pretender.get("/u/:username/staff-info.json", () => response({})); @@ -358,19 +358,11 @@ export function applyDefaultHandlers(pretender) { response(fixturesByUrl["/c/11/show.json"]) ); - pretender.get("/draft.json", (request) => { - if (request.queryParams.draft_key === "new_topic") { - return response(fixturesByUrl["/draft.json"]); - } else if (request.queryParams.draft_key.startsWith("topic_")) { - return response( - fixturesByUrl[request.url] || { - draft: null, - draft_sequence: 0, - } - ); - } - return response({}); - }); + pretender.get("/drafts.json", () => response(fixturesByUrl["/drafts.json"])); + + pretender.get("/drafts/:draft_key.json", (request) => + response(fixturesByUrl[request.url] || { draft: null, draft_sequence: 0 }) + ); pretender.get("/drafts.json", () => response(fixturesByUrl["/drafts.json"])); diff --git a/app/assets/javascripts/discourse/tests/unit/models/user-drafts-test.js b/app/assets/javascripts/discourse/tests/unit/models/user-drafts-test.js index 118089d6446..55d7e781eba 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/user-drafts-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/user-drafts-test.js @@ -7,14 +7,10 @@ import UserDraft from "discourse/models/user-draft"; module("Unit | Model | user-draft", function () { test("stream", function (assert) { const user = User.create({ id: 1, username: "eviltrout" }); - const stream = user.get("userDraftsStream"); + const stream = user.userDraftsStream; assert.present(stream, "a user has a drafts stream by default"); - assert.equal( - stream.get("itemsLoaded"), - 0, - "no items are loaded by default" - ); - assert.blank(stream.get("content"), "no content by default"); + assert.equal(stream.content.length, 0, "no items are loaded by default"); + assert.blank(stream.content, "no content by default"); }); test("draft", function (assert) { @@ -30,7 +26,7 @@ module("Unit | Model | user-draft", function () { assert.equal(drafts.length, 2, "drafts count is right"); assert.equal( - drafts[1].get("draftType"), + drafts[1].draftType, I18n.t("drafts.new_topic"), "loads correct draftType label" ); diff --git a/app/controllers/draft_controller.rb b/app/controllers/draft_controller.rb deleted file mode 100644 index 3cc77a32914..00000000000 --- a/app/controllers/draft_controller.rb +++ /dev/null @@ -1,86 +0,0 @@ -# frozen_string_literal: true - -class DraftController < ApplicationController - requires_login - - skip_before_action :check_xhr, :preload_json - - def show - raise Discourse::NotFound.new if params[:draft_key].blank? - - seq = params[:sequence] || DraftSequence.current(current_user, params[:draft_key]) - render json: { draft: Draft.get(current_user, params[:draft_key], seq), draft_sequence: seq } - end - - def update - raise Discourse::NotFound.new if params[:draft_key].blank? - - sequence = - begin - Draft.set( - current_user, - params[:draft_key], - params[:sequence].to_i, - params[:data], - params[:owner], - force_save: params[:force_save] - ) - rescue Draft::OutOfSequence - - begin - if !Draft.exists?(user_id: current_user.id, draft_key: params[:draft_key]) - Draft.set( - current_user, - params[:draft_key], - DraftSequence.current(current_user, params[:draft_key]), - params[:data], - params[:owner] - ) - else - raise Draft::OutOfSequence - end - - rescue Draft::OutOfSequence - render_json_error I18n.t('draft.sequence_conflict_error.title'), - status: 409, - extras: { - description: I18n.t('draft.sequence_conflict_error.description') - } - return - end - end - - json = success_json.merge(draft_sequence: sequence) - - begin - data = JSON::parse(params[:data]) - rescue JSON::ParserError - raise Discourse::InvalidParameters.new(:data) - end - - if data.present? - # this is a bit of a kludge we need to remove (all the parsing) too many special cases here - # we need to catch action edit and action editSharedDraft - if data["postId"].present? && data["originalText"].present? && data["action"].to_s.start_with?("edit") - post = Post.find_by(id: data["postId"]) - if post && post.raw != data["originalText"] - conflict_user = BasicUserSerializer.new(post.last_editor, root: false) - render json: json.merge(conflict_user: conflict_user) - return - end - end - end - - render json: json - end - - def destroy - begin - Draft.clear(current_user, params[:draft_key], params[:sequence].to_i) - rescue Draft::OutOfSequence - # nothing really we can do here, if try clearing a draft that is not ours, just skip it. - end - render json: success_json - end - -end diff --git a/app/controllers/drafts_controller.rb b/app/controllers/drafts_controller.rb index db6f87782ab..8ed7e539b3d 100644 --- a/app/controllers/drafts_controller.rb +++ b/app/controllers/drafts_controller.rb @@ -6,29 +6,96 @@ class DraftsController < ApplicationController skip_before_action :check_xhr, :preload_json def index - params.require(:username) params.permit(:offset) params.permit(:limit) - user = fetch_user_from_params - - unless user == current_user - raise Discourse::InvalidAccess - end - - opts = { - user: user, + stream = Draft.stream( + user: current_user, offset: params[:offset], limit: params[:limit] - } - - stream = Draft.stream(opts) + ) render json: { drafts: stream ? serialize_data(stream, DraftSerializer) : [], no_results_help: I18n.t("user_activity.no_drafts.self") } - end + def show + raise Discourse::NotFound.new if params[:id].blank? + + seq = params[:sequence] || DraftSequence.current(current_user, params[:id]) + render json: { draft: Draft.get(current_user, params[:id], seq), draft_sequence: seq } + end + + def create + raise Discourse::NotFound.new if params[:draft_key].blank? + + sequence = + begin + Draft.set( + current_user, + params[:draft_key], + params[:sequence].to_i, + params[:data], + params[:owner], + force_save: params[:force_save] + ) + rescue Draft::OutOfSequence + + begin + if !Draft.exists?(user_id: current_user.id, draft_key: params[:draft_key]) + Draft.set( + current_user, + params[:draft_key], + DraftSequence.current(current_user, params[:draft_key]), + params[:data], + params[:owner] + ) + else + raise Draft::OutOfSequence + end + + rescue Draft::OutOfSequence + render_json_error I18n.t('draft.sequence_conflict_error.title'), + status: 409, + extras: { + description: I18n.t('draft.sequence_conflict_error.description') + } + return + end + end + + json = success_json.merge(draft_sequence: sequence) + + begin + data = JSON::parse(params[:data]) + rescue JSON::ParserError + raise Discourse::InvalidParameters.new(:data) + end + + if data.present? + # this is a bit of a kludge we need to remove (all the parsing) too many special cases here + # we need to catch action edit and action editSharedDraft + if data["postId"].present? && data["originalText"].present? && data["action"].to_s.start_with?("edit") + post = Post.find_by(id: data["postId"]) + if post && post.raw != data["originalText"] + conflict_user = BasicUserSerializer.new(post.last_editor, root: false) + render json: json.merge(conflict_user: conflict_user) + return + end + end + end + + render json: json + end + + def destroy + begin + Draft.clear(current_user, params[:id], params[:sequence].to_i) + rescue Draft::OutOfSequence + # nothing really we can do here, if try clearing a draft that is not ours, just skip it. + end + render json: success_json + end end diff --git a/app/models/draft.rb b/app/models/draft.rb index 924c987edcf..459ddea41c7 100644 --- a/app/models/draft.rb +++ b/app/models/draft.rb @@ -161,7 +161,11 @@ class Draft < ActiveRecord::Base end def parsed_data - JSON.parse(data) + begin + JSON.parse(data) + rescue JSON::ParserError + {} + end end def topic_id diff --git a/config/routes.rb b/config/routes.rb index 63b1498aed4..5a4b246d1a4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -887,10 +887,7 @@ Discourse::Application.routes.draw do get "message-bus/poll" => "message_bus#poll" - resources :drafts, only: [:index] - get "draft" => "draft#show" - post "draft" => "draft#update" - delete "draft" => "draft#destroy" + resources :drafts, only: [:index, :create, :show, :destroy] if service_worker_asset = Rails.application.assets_manifest.assets['service-worker.js'] # https://developers.google.com/web/fundamentals/codelabs/debugging-service-workers/ diff --git a/spec/requests/draft_controller_spec.rb b/spec/requests/draft_controller_spec.rb deleted file mode 100644 index f828e6d89de..00000000000 --- a/spec/requests/draft_controller_spec.rb +++ /dev/null @@ -1,175 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe DraftController do - it 'requires you to be logged in' do - post "/draft" - expect(response.status).to eq(403) - end - - it 'saves a draft on update' do - user = sign_in(Fabricate(:user)) - - post "/draft.json", params: { - draft_key: 'xyz', - data: { my: "data" }.to_json, - sequence: 0 - } - - expect(response.status).to eq(200) - expect(Draft.get(user, 'xyz', 0)).to eq(%q({"my":"data"})) - end - - it "returns 404 when the key is missing" do - _user = sign_in(Fabricate(:user)) - post "/draft.json", params: { data: { my: "data" }.to_json, sequence: 0 } - expect(response.status).to eq(404) - end - - it "returns a draft if requested" do - user = sign_in(Fabricate(:user)) - Draft.set(user, 'hello', 0, 'test') - - get "/draft.json", params: { draft_key: 'hello' } - expect(response.status).to eq(200) - json = response.parsed_body - expect(json['draft']).to eq('test') - - get "/draft.json" - expect(response.status).to eq(404) - end - - it 'checks for an conflict on update' do - user = sign_in(Fabricate(:user)) - post = Fabricate(:post, user: user) - - post "/draft.json", params: { - draft_key: "topic", - sequence: 0, - data: { - postId: post.id, - originalText: post.raw, - action: "edit" - }.to_json - } - - expect(response.parsed_body['conflict_user']).to eq(nil) - - post "/draft.json", params: { - draft_key: "topic", - sequence: 0, - data: { - postId: post.id, - originalText: "something else", - action: "edit" - }.to_json - } - - json = response.parsed_body - - expect(json['conflict_user']['id']).to eq(post.last_editor.id) - expect(json['conflict_user']).to include('avatar_template') - end - - it 'destroys drafts when required' do - user = sign_in(Fabricate(:user)) - Draft.set(user, 'xxx', 0, 'hi') - delete "/draft.json", params: { draft_key: 'xxx', sequence: 0 } - expect(response.status).to eq(200) - expect(Draft.get(user, 'xxx', 0)).to eq(nil) - end - - it 'cant trivially resolve conflicts without interaction' do - - user = sign_in(Fabricate(:user)) - - DraftSequence.next!(user, "abc") - - post "/draft.json", params: { - draft_key: "abc", - sequence: 0, - data: { a: "test" }.to_json, - owner: "abcdefg" - } - - expect(response.status).to eq(200) - json = response.parsed_body - expect(json["draft_sequence"]).to eq(1) - end - - it 'has a clean protocol for ownership handover' do - user = sign_in(Fabricate(:user)) - - post "/draft.json", params: { - draft_key: "abc", - sequence: 0, - data: { a: "test" }.to_json, - owner: "abcdefg" - } - - expect(response.status).to eq(200) - - json = response.parsed_body - expect(json["draft_sequence"]).to eq(0) - - post "/draft.json", params: { - draft_key: "abc", - sequence: 0, - data: { b: "test" }.to_json, - owner: "hijklmnop" - } - - expect(response.status).to eq(200) - json = response.parsed_body - expect(json["draft_sequence"]).to eq(1) - - expect(DraftSequence.current(user, "abc")).to eq(1) - - post "/draft.json", params: { - draft_key: "abc", - sequence: 1, - data: { c: "test" }.to_json, - owner: "hijklmnop" - } - - expect(response.status).to eq(200) - json = response.parsed_body - expect(json["draft_sequence"]).to eq(2) - - post "/draft.json", params: { - draft_key: "abc", - sequence: 2, - data: { c: "test" }.to_json, - owner: "abc" - } - - expect(response.status).to eq(200) - json = response.parsed_body - expect(json["draft_sequence"]).to eq(3) - end - - it 'raises an error for out-of-sequence draft setting' do - - user = sign_in(Fabricate(:user)) - seq = DraftSequence.next!(user, "abc") - Draft.set(user, "abc", seq, { b: "test" }.to_json) - - post "/draft.json", params: { - draft_key: "abc", - sequence: seq - 1, - data: { a: "test" }.to_json - } - - expect(response.status).to eq(409) - - post "/draft.json", params: { - draft_key: "abc", - sequence: seq + 1, - data: { a: "test" }.to_json - } - - expect(response.status).to eq(409) - - end -end diff --git a/spec/requests/drafts_controller_spec.rb b/spec/requests/drafts_controller_spec.rb index 0ce0be86638..3afa76127ef 100644 --- a/spec/requests/drafts_controller_spec.rb +++ b/spec/requests/drafts_controller_spec.rb @@ -3,55 +3,210 @@ require 'rails_helper' describe DraftsController do - it 'requires you to be logged in' do - get "/drafts.json" - expect(response.status).to eq(403) + describe "#index" do + it 'requires you to be logged in' do + get "/drafts.json" + expect(response.status).to eq(403) + end + + it 'returns correct stream length after adding a draft' do + user = sign_in(Fabricate(:user)) + Draft.set(user, 'xxx', 0, '{}') + get "/drafts.json" + expect(response.status).to eq(200) + parsed = response.parsed_body + expect(response.parsed_body["drafts"].length).to eq(1) + end + + it 'has empty stream after deleting last draft' do + user = sign_in(Fabricate(:user)) + Draft.set(user, 'xxx', 0, '{}') + Draft.clear(user, 'xxx', 0) + get "/drafts.json" + expect(response.status).to eq(200) + expect(response.parsed_body["drafts"].length).to eq(0) + end + + it 'does not include topic details when user cannot see topic' do + topic = Fabricate(:private_message_topic) + topic_user = topic.user + other_user = Fabricate(:user) + Draft.set(topic_user, "topic_#{topic.id}", 0, '{}') + Draft.set(other_user, "topic_#{topic.id}", 0, '{}') + + sign_in(topic_user) + get "/drafts.json" + expect(response.status).to eq(200) + expect(response.parsed_body["drafts"].first["title"]).to eq(topic.title) + + sign_in(other_user) + get "/drafts.json" + expect(response.status).to eq(200) + expect(response.parsed_body["drafts"].first["title"]).to eq(nil) + end end - it 'returns correct stream length after adding a draft' do - user = sign_in(Fabricate(:user)) - Draft.set(user, 'xxx', 0, '{}') - get "/drafts.json", params: { username: user.username } - expect(response.status).to eq(200) - parsed = response.parsed_body - expect(parsed["drafts"].length).to eq(1) + describe "#show" do + it "returns a draft if requested" do + user = sign_in(Fabricate(:user)) + Draft.set(user, 'hello', 0, 'test') + + get "/drafts/hello.json" + expect(response.status).to eq(200) + expect(response.parsed_body['draft']).to eq('test') + end end - it 'has empty stream after deleting last draft' do - user = sign_in(Fabricate(:user)) - Draft.set(user, 'xxx', 0, '{}') - Draft.clear(user, 'xxx', 0) - get "/drafts.json", params: { username: user.username } - expect(response.status).to eq(200) - parsed = response.parsed_body - expect(parsed["drafts"].length).to eq(0) + describe "#create" do + it 'requires you to be logged in' do + post "/drafts.json" + expect(response.status).to eq(403) + end + + it 'saves a draft' do + user = sign_in(Fabricate(:user)) + + post "/drafts.json", params: { + draft_key: 'xyz', + data: { my: "data" }.to_json, + sequence: 0 + } + + expect(response.status).to eq(200) + expect(Draft.get(user, 'xyz', 0)).to eq(%q({"my":"data"})) + end + + it "returns 404 when the key is missing" do + sign_in(Fabricate(:user)) + post "/drafts.json", params: { data: { my: "data" }.to_json, sequence: 0 } + expect(response.status).to eq(404) + end + + it 'checks for an conflict on update' do + user = sign_in(Fabricate(:user)) + post = Fabricate(:post, user: user) + + post "/drafts.json", params: { + draft_key: "topic", + sequence: 0, + data: { + postId: post.id, + originalText: post.raw, + action: "edit" + }.to_json + } + + expect(response.status).to eq(200) + expect(response.parsed_body['conflict_user']).to eq(nil) + + post "/drafts.json", params: { + draft_key: "topic", + sequence: 0, + data: { + postId: post.id, + originalText: "something else", + action: "edit" + }.to_json + } + + expect(response.status).to eq(200) + expect(response.parsed_body['conflict_user']['id']).to eq(post.last_editor.id) + expect(response.parsed_body['conflict_user']).to include('avatar_template') + end + + it 'cant trivially resolve conflicts without interaction' do + + user = sign_in(Fabricate(:user)) + + DraftSequence.next!(user, "abc") + + post "/drafts.json", params: { + draft_key: "abc", + sequence: 0, + data: { a: "test" }.to_json, + owner: "abcdefg" + } + + expect(response.status).to eq(200) + expect(response.parsed_body["draft_sequence"]).to eq(1) + end + + it 'has a clean protocol for ownership handover' do + user = sign_in(Fabricate(:user)) + + post "/drafts.json", params: { + draft_key: "abc", + sequence: 0, + data: { a: "test" }.to_json, + owner: "abcdefg" + } + + expect(response.status).to eq(200) + expect(response.parsed_body["draft_sequence"]).to eq(0) + + post "/drafts.json", params: { + draft_key: "abc", + sequence: 0, + data: { b: "test" }.to_json, + owner: "hijklmnop" + } + + expect(response.status).to eq(200) + expect(response.parsed_body["draft_sequence"]).to eq(1) + + expect(DraftSequence.current(user, "abc")).to eq(1) + + post "/drafts.json", params: { + draft_key: "abc", + sequence: 1, + data: { c: "test" }.to_json, + owner: "hijklmnop" + } + + expect(response.status).to eq(200) + expect(response.parsed_body["draft_sequence"]).to eq(2) + + post "/drafts.json", params: { + draft_key: "abc", + sequence: 2, + data: { c: "test" }.to_json, + owner: "abc" + } + + expect(response.status).to eq(200) + expect(response.parsed_body["draft_sequence"]).to eq(3) + end + + it 'raises an error for out-of-sequence draft setting' do + user = sign_in(Fabricate(:user)) + seq = DraftSequence.next!(user, "abc") + Draft.set(user, "abc", seq, { b: "test" }.to_json) + + post "/drafts.json", params: { + draft_key: "abc", + sequence: seq - 1, + data: { a: "test" }.to_json + } + + expect(response.status).to eq(409) + + post "/drafts.json", params: { + draft_key: "abc", + sequence: seq + 1, + data: { a: "test" }.to_json + } + + expect(response.status).to eq(409) + end end - it 'does not let a user see drafts stream of another user' do - user_b = Fabricate(:user) - Draft.set(user_b, 'xxx', 0, '{}') - sign_in(Fabricate(:user)) - get "/drafts.json", params: { username: user_b.username } - expect(response.status).to eq(403) - end - - it 'does not include topic details when user cannot see topic' do - topic = Fabricate(:private_message_topic) - topic_user = topic.user - other_user = Fabricate(:user) - Draft.set(topic_user, "topic_#{topic.id}", 0, '{}') - Draft.set(other_user, "topic_#{topic.id}", 0, '{}') - - sign_in(topic_user) - get "/drafts.json", params: { username: topic_user.username } - expect(response.status).to eq(200) - parsed = response.parsed_body - expect(parsed["drafts"].first["title"]).to eq(topic.title) - - sign_in(other_user) - get "/drafts.json", params: { username: other_user.username } - expect(response.status).to eq(200) - parsed = response.parsed_body - expect(parsed["drafts"].first["title"]).to eq(nil) + describe "#destroy" do + it 'destroys drafts when required' do + user = sign_in(Fabricate(:user)) + Draft.set(user, 'xxx', 0, 'hi') + delete "/drafts/xxx.json", params: { sequence: 0 } + expect(response.status).to eq(200) + expect(Draft.get(user, 'xxx', 0)).to eq(nil) + end end end diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 74fce7879a6..18042a9efa2 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -478,7 +478,7 @@ RSpec.describe SessionController do UserAuthToken.destroy_all # we need a route that will call current user - post '/draft.json', params: {} + post '/drafts.json', params: {} expect(response.headers['Discourse-Logged-Out']).to eq("1") end end