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

Add Debates engine #9

Merged
merged 16 commits into from
Jan 27, 2017
Merged

Add Debates engine #9

merged 16 commits into from
Jan 27, 2017

Conversation

ItsGenis
Copy link
Contributor

@ItsGenis ItsGenis commented Jan 18, 2017

🎩 What? Why?

Fixes #3

👻 GIF

@@ -0,0 +1,2 @@
ENV["ENGINE_NAME"] = File.dirname(File.dirname(__FILE__)).split("/").last

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing frozen string literal comment.

it "creates a new meeting" do
find(".actions .new").click

within ".new_meeting" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [41/25]

end
end

it "creates a new meeting" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [50/25]

@@ -0,0 +1,121 @@
# -*- coding: utf-8 -*-
# frozen_string_literal: true
RSpec.shared_examples "manage meetings" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [99/25]

@@ -0,0 +1,11 @@
RSpec.shared_context "admin" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing frozen string literal comment.

let(:params) { default_params.merge(scope_id: [scope2.id, scope1.id]) }

it "filters meetings by scope" do
expect(subject.results).to match_array [meeting1,meeting2]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing after comma.

end
end

describe "filters" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [56/25]

let(:default_params) { { feature: nil } }

it "raises an error" do
expect{ subject.results }.to raise_error(StandardError, "Missing feature")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing to the left of {.

@@ -0,0 +1,135 @@
require "spec_helper"

describe Decidim::Meetings::MeetingSearch do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [108/25]

@@ -0,0 +1,135 @@
require "spec_helper"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing frozen string literal comment.


require "spec_helper"

describe Decidim::Meetings::Admin::MeetingForm do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [99/25]

@@ -0,0 +1,133 @@
# frozen_literal_string: true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing frozen string literal comment.

end
end

context "show" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [61/25]

expect(page).to have_selector("article.card", count: meetings_count)

meetings.each do |meeting|
expect(page).to have_content(translated meeting.title)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add parentheses to nested method call translated meeting.title.

@@ -0,0 +1,107 @@
require "spec_helper"

describe "Explore meetings", type: :feature do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [89/25]

@@ -0,0 +1,107 @@
require "spec_helper"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing frozen string literal comment.

@ItsGenis ItsGenis force-pushed the feature/debates-engine branch from 540aa99 to 8b02ad5 Compare January 23, 2017 09:37
it "creates a new debate" do
find(".actions .new").click

within ".new_debate" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [40/25]

end
end

it "creates a new debate" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [49/25]

@@ -0,0 +1,120 @@
# -*- coding: utf-8 -*-
# frozen_string_literal: true
RSpec.shared_examples "manage debates" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [98/25]

end
end

describe "filters" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [35/25]

let(:default_params) { { feature: nil } }

it "raises an error" do
expect{ subject.results }.to raise_error(StandardError, "Missing feature")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing to the left of {.

@@ -0,0 +1,104 @@
require "spec_helper"

describe Decidim::Debates::DebateSearch do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [83/25]

@@ -0,0 +1,104 @@
require "spec_helper"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing frozen string literal comment.


require "spec_helper"

describe Decidim::Debates::Admin::DebateForm do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block has too many lines. [99/25]

@@ -0,0 +1,133 @@
# frozen_literal_string: true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing frozen string literal comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of this things is not like the other

@@ -0,0 +1,12 @@
require "decidim/core/test/factories"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing frozen string literal comment.

description: {en: "description"},
short_description: {en: "short_description"},
location: {en: "location"},
location_hints: {en: "location_hints"},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.

title: {en: "title"},
description: {en: "description"},
short_description: {en: "short_description"},
location: {en: "location"},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.

:invalid? => invalid,
title: {en: "title"},
description: {en: "description"},
short_description: {en: "short_description"},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space inside { missing.
Space inside } missing.

@ItsGenis ItsGenis force-pushed the feature/debates-engine branch 2 times, most recently from 0d51b5e to 41e83d0 Compare January 27, 2017 14:04
@ItsGenis ItsGenis changed the title Add Debates engine [WIP] Add Debates engine Jan 27, 2017
@@ -1,3 +1,4 @@
# frozen_string_literal: true
# frozen_literal_string: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this one


helper_method :debates, :debate

def index; end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need if there's a view named index

validates :current_feature, presence: true
validates :category, presence: true, if: ->(form) { form.decidim_category_id.present? }

def scope
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we validate the scope like category?

Copy link
Contributor Author

@ItsGenis ItsGenis Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has no scope, this has to be removed.

validates :start_time, presence: true, date: { before: :end_time }
validates :end_time, presence: true, date: { after: :start_time }

validates :current_feature, presence: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current feature is automatically injected, no need to validate.

validate :author_belongs_to_organization
validates :title, presence: true

def author_name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the author name from here & the views. Debates are only created by admins so it makes no sense.

user_group&.name || author&.name || I18n.t("decidim.debates.models.debate.fields.official_debate")
end

def author_avatar_url
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

</div>
<div class="author-data__extra">
<a href="#comments" title="<%= t('.comments') %>">
<%= icon "comment-square", aria_label: t('.comments'), role: "img" %> <%= Decidim::Comments::Comment.where(commentable: debate).count %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a relation at debate, something like:

has_many :comments, as: :commentable

So you can just do debate.comments.count

/cc @josepjaume @beagleknight

@@ -0,0 +1,18 @@
# This migration comes from decidim_debates (originally 20170118141619)
class CreateDebates < ActiveRecord::Migration[5.0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file, it belongs to the engine.

s.description = s.summary
s.version = "0.0.1"
s.authors = ["Josep Jaume Rey Peroy", "Marc Riera Casals", "Oriol Gual Oliva", "Genís Matutes Pujol"]
s.email = ["josepjaume@gmail.com", "mrc2407@gmail.com", "oriolgual@gmail.com", "genis.matutes@gmail.com"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version, authors & email aren't necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually, wrong? Can't remember when but I was forced to put them in to use the engine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not wrong but we've omitted these so it "inherits" it from the main gem, which is the only one published.


s.files = Dir["{app,config,db,lib}/**/*", "Rakefile", "README.md"]

s.add_dependency "decidim-core"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s.add_dependency "decidim-core", Decidim.version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comments too:
s.add_dependency "decidim-comments", Decidim.version

@@ -0,0 +1,133 @@
# frozen_literal_string: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of this things is not like the other

@ItsGenis ItsGenis force-pushed the feature/debates-engine branch 2 times, most recently from a7fee0c to ceb4102 Compare January 27, 2017 14:45
@ItsGenis ItsGenis requested a review from beagleknight January 27, 2017 14:45
@ItsGenis ItsGenis force-pushed the feature/debates-engine branch from ceb4102 to d51c5a2 Compare January 27, 2017 14:48
@ItsGenis ItsGenis force-pushed the feature/debates-engine branch from a39547c to ddfd257 Compare January 27, 2017 15:31
@oriolgual oriolgual merged this pull request into master Jan 27, 2017
@oriolgual oriolgual deleted the feature/debates-engine branch January 27, 2017 16:01
josepjaume pushed a commit that referenced this pull request Jan 28, 2017
* Add Debates engine WIP [ci-skip]

* Remove scopes

* Bump, get tests ready

* Finish bumping unit tests

* WIP [ci-skip]

* Use real index markup

* More markup WIP [ci-skip]

* Add instructions

* Add comments and fix markup

* Add missing translations

* Pass rubocop

* Remove DS_Store files

* Remove authorable from debates

* Reviewd stuff

* Rebase

* Fix stuff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants