From 0c233e4e256426bee063b7512cc02a171f6edb7d Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 14 Apr 2015 14:21:02 -0400 Subject: [PATCH] Interface is wired up for Approving/Rejecting posts --- .../discourse/adapters/rest.js.es6 | 21 ++++++-- .../discourse/controllers/queued-posts.js.es6 | 24 ++++----- .../discourse/lib/ajax-error.js.es6 | 54 +++++++++++-------- .../javascripts/discourse/models/rest.js.es6 | 11 +++- .../javascripts/discourse/models/store.js.es6 | 37 +++++++++++-- .../discourse/routes/queued-posts.js.es6 | 1 - .../discourse/routes/topic-from-params.js.es6 | 3 ++ .../discourse/templates/queued-posts.hbs | 54 ++++++++++++------- .../stylesheets/desktop/queued-posts.scss | 6 ++- app/controllers/application_controller.rb | 1 + app/controllers/queued_posts_controller.rb | 12 ++++- app/serializers/queued_post_serializer.rb | 3 ++ config/locales/client.en.yml | 1 + .../queued_posts_controller_spec.rb | 31 +++++++++++ spec/fabricators/queued_post_fabricator.rb | 18 +++++++ spec/models/queued_post_spec.rb | 17 +----- test/javascripts/acceptance/topic-test.js.es6 | 2 +- .../helpers/create-pretender.js.es6 | 8 +++ .../javascripts/models/rest-model-test.js.es6 | 51 ++++++++++++++++-- test/javascripts/models/store-test.js.es6 | 8 +++ 20 files changed, 273 insertions(+), 90 deletions(-) create mode 100644 spec/fabricators/queued_post_fabricator.rb diff --git a/app/assets/javascripts/discourse/adapters/rest.js.es6 b/app/assets/javascripts/discourse/adapters/rest.js.es6 index bd566123540..ec25f2d1341 100644 --- a/app/assets/javascripts/discourse/adapters/rest.js.es6 +++ b/app/assets/javascripts/discourse/adapters/rest.js.es6 @@ -6,6 +6,16 @@ export function Result(payload, responseJson) { this.target = null; } +const ajax = Discourse.ajax; + +// We use this to make sure 404s are caught +function rethrow(error) { + if (error.status === 404) { + throw "404: " + error.responseText; + } + throw(error); +} + export default Ember.Object.extend({ pathFor(store, type, findArgs) { let path = "/" + Ember.String.underscore(store.pluralize(type)); @@ -31,17 +41,18 @@ export default Ember.Object.extend({ }, findAll(store, type) { - return Discourse.ajax(this.pathFor(store, type)); + return ajax(this.pathFor(store, type)).catch(rethrow); }, + find(store, type, findArgs) { - return Discourse.ajax(this.pathFor(store, type, findArgs)); + return ajax(this.pathFor(store, type, findArgs)).catch(rethrow); }, update(store, type, id, attrs) { const data = {}; data[Ember.String.underscore(type)] = attrs; - return Discourse.ajax(this.pathFor(store, type, id), { method: 'PUT', data }).then(function(json) { + return ajax(this.pathFor(store, type, id), { method: 'PUT', data }).then(function(json) { return new Result(json[type], json); }); }, @@ -50,13 +61,13 @@ export default Ember.Object.extend({ const data = {}; const typeField = Ember.String.underscore(type); data[typeField] = attrs; - return Discourse.ajax(this.pathFor(store, type), { method: 'POST', data }).then(function (json) { + return ajax(this.pathFor(store, type), { method: 'POST', data }).then(function (json) { return new Result(json[typeField], json); }); }, destroyRecord(store, type, record) { - return Discourse.ajax(this.pathFor(store, type, record.get('id')), { method: 'DELETE' }); + return ajax(this.pathFor(store, type, record.get('id')), { method: 'DELETE' }); } }); diff --git a/app/assets/javascripts/discourse/controllers/queued-posts.js.es6 b/app/assets/javascripts/discourse/controllers/queued-posts.js.es6 index a597c2cb40b..90939e3b712 100644 --- a/app/assets/javascripts/discourse/controllers/queued-posts.js.es6 +++ b/app/assets/javascripts/discourse/controllers/queued-posts.js.es6 @@ -1,16 +1,16 @@ +import { popupAjaxError } from 'discourse/lib/ajax-error'; + +function updateState(state) { + return function(post) { + post.update({ state }).then(() => { + this.get('model').removeObject(post); + }).catch(popupAjaxError); + }; +} + export default Ember.Controller.extend({ - actions: { - approve(post) { - post.update({ state: 'approved' }).then(() => { - this.get('model').removeObject(post); - }); - }, - - reject(post) { - post.update({ state: 'rejected' }).then(() => { - this.get('model').removeObject(post); - }); - } + approve: updateState('approved'), + reject: updateState('rejected') } }); diff --git a/app/assets/javascripts/discourse/lib/ajax-error.js.es6 b/app/assets/javascripts/discourse/lib/ajax-error.js.es6 index 0117c94a5ba..f1a34add676 100644 --- a/app/assets/javascripts/discourse/lib/ajax-error.js.es6 +++ b/app/assets/javascripts/discourse/lib/ajax-error.js.es6 @@ -1,30 +1,38 @@ +function extractError(error) { + if (error instanceof Error) { + Ember.Logger.error(error.stack); + } + + if (typeof error === "string") { + Ember.Logger.error(error); + } + + let parsedError; + if (error.responseText) { + try { + const parsedJSON = $.parseJSON(error.responseText); + if (parsedJSON.errors) { + parsedError = parsedJSON.errors[0]; + } else if (parsedJSON.failed) { + parsedError = parsedJSON.message; + } + } catch(ex) { + // in case the JSON doesn't parse + Ember.Logger.error(ex.stack); + } + } + return parsedError || I18n.t('generic_error'); +} + export function throwAjaxError(undoCallback) { return function(error) { - if (error instanceof Error) { - Ember.Logger.error(error.stack); - } - - if (typeof error === "string") { - Ember.Logger.error(error); - } - // If we provided an `undo` callback if (undoCallback) { undoCallback(error); } - let parsedError; - if (error.responseText) { - try { - const parsedJSON = $.parseJSON(error.responseText); - if (parsedJSON.errors) { - parsedError = parsedJSON.errors[0]; - } else if (parsedJSON.failed) { - parsedError = parsedJSON.message; - } - } catch(ex) { - // in case the JSON doesn't parse - Ember.Logger.error(ex.stack); - } - } - throw parsedError || I18n.t('generic_error'); + throw extractError(error); }; } + +export function popupAjaxError(err) { + bootbox.alert(extractError(err)); +} diff --git a/app/assets/javascripts/discourse/models/rest.js.es6 b/app/assets/javascripts/discourse/models/rest.js.es6 index a35da6edf41..8bb5d0250f3 100644 --- a/app/assets/javascripts/discourse/models/rest.js.es6 +++ b/app/assets/javascripts/discourse/models/rest.js.es6 @@ -3,24 +3,30 @@ import Presence from 'discourse/mixins/presence'; const RestModel = Ember.Object.extend(Presence, { isNew: Ember.computed.equal('__state', 'new'), isCreated: Ember.computed.equal('__state', 'created'), + isSaving: false, afterUpdate: Ember.K, update(props) { + if (this.get('isSaving')) { return Ember.RSVP.reject(); } + props = props || this.updateProperties(); const type = this.get('__type'), store = this.get('store'); const self = this; + self.set('isSaving', true); return store.update(type, this.get('id'), props).then(function(res) { self.setProperties(self.__munge(res.payload || res.responseJson)); self.afterUpdate(res); return res; - }); + }).finally(() => this.set('isSaving', false)); }, _saveNew(props) { + if (this.get('isSaving')) { return Ember.RSVP.reject(); } + props = props || this.createProperties(); const type = this.get('__type'), @@ -28,6 +34,7 @@ const RestModel = Ember.Object.extend(Presence, { adapter = store.adapterFor(type); const self = this; + self.set('isSaving', true); return adapter.createRecord(store, type, props).then(function(res) { if (!res) { throw "Received no data back from createRecord"; } @@ -40,7 +47,7 @@ const RestModel = Ember.Object.extend(Presence, { res.target = self; return res; - }); + }).finally(() => this.set('isSaving', false)); }, createProperties() { diff --git a/app/assets/javascripts/discourse/models/store.js.es6 b/app/assets/javascripts/discourse/models/store.js.es6 index e54eaee4a21..61ce9f581c8 100644 --- a/app/assets/javascripts/discourse/models/store.js.es6 +++ b/app/assets/javascripts/discourse/models/store.js.es6 @@ -36,7 +36,7 @@ export default Ember.Object.extend({ if (typeof findArgs === "object") { return self._resultSet(type, result); } else { - return self._hydrate(type, result[Ember.String.underscore(type)]); + return self._hydrate(type, result[Ember.String.underscore(type)], result); } }); }, @@ -48,7 +48,7 @@ export default Ember.Object.extend({ const typeName = Ember.String.underscore(self.pluralize(type)), totalRows = result["total_rows_" + typeName] || result.get('totalRows'), loadMoreUrl = result["load_more_" + typeName], - content = result[typeName].map(obj => self._hydrate(type, obj)); + content = result[typeName].map(obj => self._hydrate(type, obj, result)); resultSet.setProperties({ totalRows, loadMoreUrl }); resultSet.get('content').pushObjects(content); @@ -86,7 +86,7 @@ export default Ember.Object.extend({ _resultSet(type, result) { const typeName = Ember.String.underscore(this.pluralize(type)), - content = result[typeName].map(obj => this._hydrate(type, obj)), + content = result[typeName].map(obj => this._hydrate(type, obj, result)), totalRows = result["total_rows_" + typeName] || content.length, loadMoreUrl = result["load_more_" + typeName]; @@ -111,10 +111,39 @@ export default Ember.Object.extend({ return this.container.lookup('adapter:' + type) || this.container.lookup('adapter:rest'); }, - _hydrate(type, obj) { + _hydrateEmbedded(obj, root) { + const self = this; + Object.keys(obj).forEach(function(k) { + const m = /(.+)\_id$/.exec(k); + if (m) { + const subType = m[1]; + const collection = root[self.pluralize(subType)]; + if (collection) { + const found = collection.findProperty('id', obj[k]); + if (found) { + const hydrated = self._hydrate(subType, found, root); + if (hydrated) { + obj[subType] = hydrated; + delete obj[k]; + } + } + } + } + }); + }, + + _hydrate(type, obj, root) { if (!obj) { throw "Can't hydrate " + type + " of `null`"; } if (!obj.id) { throw "Can't hydrate " + type + " without an `id`"; } + root = root || obj; + + // Experimental: If serialized with a certain option we'll wire up embedded objects + // automatically. + if (root.__rest_serializer === "1") { + this._hydrateEmbedded(obj, root); + } + _identityMap[type] = _identityMap[type] || {}; const existing = _identityMap[type][obj.id]; diff --git a/app/assets/javascripts/discourse/routes/queued-posts.js.es6 b/app/assets/javascripts/discourse/routes/queued-posts.js.es6 index 58b28dd8fa8..700b9871a57 100644 --- a/app/assets/javascripts/discourse/routes/queued-posts.js.es6 +++ b/app/assets/javascripts/discourse/routes/queued-posts.js.es6 @@ -5,4 +5,3 @@ export default DiscourseRoute.extend({ return this.store.find('queuedPost', {status: 'new'}); } }); - diff --git a/app/assets/javascripts/discourse/routes/topic-from-params.js.es6 b/app/assets/javascripts/discourse/routes/topic-from-params.js.es6 index 9c414daac03..62dd9e5dbfa 100644 --- a/app/assets/javascripts/discourse/routes/topic-from-params.js.es6 +++ b/app/assets/javascripts/discourse/routes/topic-from-params.js.es6 @@ -1,6 +1,9 @@ // This route is used for retrieving a topic based on params export default Discourse.Route.extend({ + // Avoid default model hook + model: function() { return; }, + setupController: function(controller, params) { params = params || {}; params.track_visit = true; diff --git a/app/assets/javascripts/discourse/templates/queued-posts.hbs b/app/assets/javascripts/discourse/templates/queued-posts.hbs index 2119de912b0..e2d096514c2 100644 --- a/app/assets/javascripts/discourse/templates/queued-posts.hbs +++ b/app/assets/javascripts/discourse/templates/queued-posts.hbs @@ -1,28 +1,44 @@
{{#each post in model}} -
- {{#if post.title}} -

{{post.title}}

- {{/if}} -
- {{avatar post.user imageSize="large"}} -
-
-
- {{post.user.username}} +
+
+ {{avatar post.user imageSize="large"}} +
+
+
+ {{post.user.username}} +
+
+ + + {{i18n "queue.topic"}} + {{#if post.topic}} + {{topic-link post.topic}} + {{else}} + {{post.post_options.title}} + {{/if}} + + + {{{cook-text post.raw}}} + +
+ {{d-button action="approve" + actionParam=post + disabled=post.isSaving + label="queue.approve" + icon="check" + class="btn-primary approve"}} + {{d-button action="reject" + actionParam=post + disabled=post.isSaving + label="queue.reject" + icon="times" + class="btn-warning reject"}} +
- - {{{cook-text post.raw}}} - -
- {{d-button action="approve" actionParam=post label="queue.approve" icon="check" class="btn-primary approve"}} - {{d-button action="reject" actionParam=post label="queue.reject" icon="times" class="btn-warning reject"}} -
-
-
{{else}}

{{i18n "queue.none"}}

{{/each}} diff --git a/app/assets/stylesheets/desktop/queued-posts.scss b/app/assets/stylesheets/desktop/queued-posts.scss index 50955c9970f..49cdb433de1 100644 --- a/app/assets/stylesheets/desktop/queued-posts.scss +++ b/app/assets/stylesheets/desktop/queued-posts.scss @@ -10,8 +10,10 @@ width: $topic-body-width; float: left; } - h4.title { - margin-bottom: 1em; + + .post-title { + color: darken(scale-color-diff(), 50%); + font-weight: bold; } border-bottom: 1px solid darken(scale-color-diff(), 10%); diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7ee38765d2a..146fe5771b1 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -219,6 +219,7 @@ class ApplicationController < ActionController::Base def render_json_dump(obj, opts=nil) opts ||= {} + obj['__rest_serializer'] = "1" if opts[:rest_serializer] render json: MultiJson.dump(obj), status: opts[:status] || 200 end diff --git a/app/controllers/queued_posts_controller.rb b/app/controllers/queued_posts_controller.rb index e7dd20d79da..8ee84538eb9 100644 --- a/app/controllers/queued_posts_controller.rb +++ b/app/controllers/queued_posts_controller.rb @@ -8,12 +8,20 @@ class QueuedPostsController < ApplicationController state = QueuedPost.states[(params[:state] || 'new').to_sym] state ||= QueuedPost.states[:new] - @queued_posts = QueuedPost.where(state: state) - render_serialized(@queued_posts, QueuedPostSerializer, root: :queued_posts) + @queued_posts = QueuedPost.where(state: state).includes(:topic, :user) + render_serialized(@queued_posts, QueuedPostSerializer, root: :queued_posts, rest_serializer: true) end def update qp = QueuedPost.where(id: params[:id]).first + + state = params[:queued_post][:state] + if state == 'approved' + qp.approve!(current_user) + elsif state == 'rejected' + qp.reject!(current_user) + end + render_serialized(qp, QueuedPostSerializer, root: :queued_posts) end diff --git a/app/serializers/queued_post_serializer.rb b/app/serializers/queued_post_serializer.rb index ed9b9a70036..8e2637bce7a 100644 --- a/app/serializers/queued_post_serializer.rb +++ b/app/serializers/queued_post_serializer.rb @@ -1,4 +1,5 @@ class QueuedPostSerializer < ApplicationSerializer + attributes :id, :queue, :user_id, @@ -11,4 +12,6 @@ class QueuedPostSerializer < ApplicationSerializer :created_at has_one :user, serializer: BasicUserSerializer, embed: :object + has_one :topic, serializer: BasicTopicSerializer + end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d27df473215..f6a1d4e8786 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -226,6 +226,7 @@ en: placeholder: "type the topic title here" queue: + topic: "Topic:" approve: 'Approve Post' reject: 'Reject Post' title: "Needs Approval" diff --git a/spec/controllers/queued_posts_controller_spec.rb b/spec/controllers/queued_posts_controller_spec.rb index 99cbd383444..1e4210797ad 100644 --- a/spec/controllers/queued_posts_controller_spec.rb +++ b/spec/controllers/queued_posts_controller_spec.rb @@ -1,4 +1,6 @@ require 'spec_helper' +require_dependency 'queued_posts_controller' +require_dependency 'queued_post' describe QueuedPostsController do context 'without authentication' do @@ -24,5 +26,34 @@ describe QueuedPostsController do expect(response).to be_success end end + + + context 'update' do + let!(:user) { log_in(:moderator) } + let(:qp) { Fabricate(:queued_post) } + + context 'approved' do + it 'updates the post to approved' do + + xhr :put, :update, id: qp.id, queued_post: { state: 'approved' } + expect(response).to be_success + + qp.reload + expect(qp.state).to eq(QueuedPost.states[:approved]) + end + end + + context 'rejected' do + it 'updates the post to approved' do + + xhr :put, :update, id: qp.id, queued_post: { state: 'rejected' } + expect(response).to be_success + + qp.reload + expect(qp.state).to eq(QueuedPost.states[:rejected]) + end + end + + end end diff --git a/spec/fabricators/queued_post_fabricator.rb b/spec/fabricators/queued_post_fabricator.rb new file mode 100644 index 00000000000..45cae4006e1 --- /dev/null +++ b/spec/fabricators/queued_post_fabricator.rb @@ -0,0 +1,18 @@ +Fabricator(:queued_post) do + queue 'new_post' + state QueuedPost.states[:new] + user + topic + raw 'This post should be queued up' + post_options do + { reply_to_post_number: 1, + via_email: true, + raw_email: 'store_me', + auto_track: true, + custom_fields: { hello: 'world' }, + cooking_options: { cat: 'hat' }, + cook_method: Post.cook_methods[:raw_html], + image_sizes: {"http://foo.bar/image.png" => {"width" => 0, "height" => 222}} } + end +end + diff --git a/spec/models/queued_post_spec.rb b/spec/models/queued_post_spec.rb index 521819fef5f..976c3faf169 100644 --- a/spec/models/queued_post_spec.rb +++ b/spec/models/queued_post_spec.rb @@ -7,22 +7,7 @@ describe QueuedPost do let(:topic) { Fabricate(:topic) } let(:user) { Fabricate(:user) } let(:admin) { Fabricate(:admin) } - let(:qp) { QueuedPost.create(queue: 'new_post', - state: QueuedPost.states[:new], - user_id: user.id, - topic_id: topic.id, - raw: 'This post should be queued up', - post_options: { - reply_to_post_number: 1, - via_email: true, - raw_email: 'store_me', - auto_track: true, - custom_fields: { hello: 'world' }, - cooking_options: { cat: 'hat' }, - cook_method: Post.cook_methods[:raw_html], - not_create_option: true, - image_sizes: {"http://foo.bar/image.png" => {"width" => 0, "height" => 222}} - }) } + let(:qp) { Fabricate(:queued_post, topic: topic, user: user) } it "returns the appropriate options for posting" do create_options = qp.create_options diff --git a/test/javascripts/acceptance/topic-test.js.es6 b/test/javascripts/acceptance/topic-test.js.es6 index 53bf0c3391b..6d4aa67fc5d 100644 --- a/test/javascripts/acceptance/topic-test.js.es6 +++ b/test/javascripts/acceptance/topic-test.js.es6 @@ -2,7 +2,7 @@ import { acceptance } from "helpers/qunit-helpers"; acceptance("View Topic"); test("Enter a Topic", () => { - visit("/t/internationalization-localization/280"); + visit("/t/internationalization-localization/280/1"); andThen(() => { ok(exists("#topic"), "The topic was rendered"); ok(exists("#topic .post-cloak"), "The topic has cloaked posts"); diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index c71f168c46d..1a4de17aa0a 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -173,6 +173,14 @@ export default function() { }); }); + this.get('/fruits/:id', function() { + return response({ + __rest_serializer: "1", + fruit: {id: 1, name: 'apple', farmer_id: 1}, + farmers: [{id: 1, name: 'Evil Trout'}] + }); + }); + this.get('/widgets/:widget_id', function(request) { const w = _widgets.findBy('id', parseInt(request.params.widget_id)); if (w) { diff --git a/test/javascripts/models/rest-model-test.js.es6 b/test/javascripts/models/rest-model-test.js.es6 index 9aa63c4c300..182532b139d 100644 --- a/test/javascripts/models/rest-model-test.js.es6 +++ b/test/javascripts/models/rest-model-test.js.es6 @@ -19,23 +19,51 @@ test('munging', function() { test('update', function() { const store = createStore(); - store.find('widget', 123).then(function(widget) { equal(widget.get('name'), 'Trout Lure'); - widget.update({ name: 'new name' }).then(function() { + + ok(!widget.get('isSaving')); + const promise = widget.update({ name: 'new name' }); + ok(widget.get('isSaving')); + promise.then(function() { + ok(!widget.get('isSaving')); equal(widget.get('name'), 'new name'); }); }); }); +test('updating simultaneously', function() { + expect(2); + + const store = createStore(); + store.find('widget', 123).then(function(widget) { + + const firstPromise = widget.update({ name: 'new name' }); + const secondPromise = widget.update({ name: 'new name' }); + firstPromise.then(function() { + ok(true, 'the first promise succeeeds'); + }); + + secondPromise.catch(function() { + ok(true, 'the second promise fails'); + }); + + }); +}); + test('save new', function() { const store = createStore(); const widget = store.createRecord('widget'); ok(widget.get('isNew'), 'it is a new record'); ok(!widget.get('isCreated'), 'it is not created'); + ok(!widget.get('isSaving')); - widget.save({ name: 'Evil Widget' }).then(function() { + const promise = widget.save({ name: 'Evil Widget' }); + ok(widget.get('isSaving')); + + promise.then(function() { + ok(!widget.get('isSaving')); ok(widget.get('id'), 'it has an id'); ok(widget.get('name'), 'Evil Widget'); ok(widget.get('isCreated'), 'it is created'); @@ -43,6 +71,23 @@ test('save new', function() { }); }); +test('creating simultaneously', function() { + expect(2); + + const store = createStore(); + const widget = store.createRecord('widget'); + + const firstPromise = widget.save({ name: 'Evil Widget' }); + const secondPromise = widget.save({ name: 'Evil Widget' }); + firstPromise.then(function() { + ok(true, 'the first promise succeeeds'); + }); + + secondPromise.catch(function() { + ok(true, 'the second promise fails'); + }); +}); + test('destroyRecord', function() { const store = createStore(); store.find('widget', 123).then(function(widget) { diff --git a/test/javascripts/models/store-test.js.es6 b/test/javascripts/models/store-test.js.es6 index 9b3893cf430..13e2e8d0041 100644 --- a/test/javascripts/models/store-test.js.es6 +++ b/test/javascripts/models/store-test.js.es6 @@ -88,3 +88,11 @@ test('destroyRecord', function() { }); }); }); + +test('find embedded', function() { + const store = createStore(); + store.find('fruit', 1).then(function(f) { + ok(f.get('farmer'), 'it has the embedded object'); + }); +}); +