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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hashtags Feature applied to Proposals #3959

Merged
merged 21 commits into from Aug 30, 2018
Merged

Hashtags Feature applied to Proposals #3959

merged 21 commits into from Aug 30, 2018

Conversation

@isaacmg410
Copy link
Contributor

@isaacmg410 isaacmg410 commented Aug 3, 2018

馃帺 What? Why?

This functionality will consist of the development of a new feature Hashtags provide a flexible way to search and filter content.

馃搶 Related Issues

馃搵 Subtasks

  • Add CHANGELOG entry
  • Add hashtags features to proposals
  • Add documentation regarding the feature
  • Add tests

馃摲 Screenshots (optional)

Description

@ghost ghost assigned isaacmg410 Aug 3, 2018
@ghost ghost added the status: WIP label Aug 3, 2018
@isaacmg410
Copy link
Contributor Author

@isaacmg410 isaacmg410 commented Aug 3, 2018

@decidim/lot-core I would appreciate a pre-review of this PR.
I will try to make more smaller PRs for a better 馃槃

@@ -0,0 +1,16 @@
# frozen_string_literal: true

class CreateDecidimHashtagggings < ActiveRecord::Migration[5.2]

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

Looks like there's an extra g here

description "hashtags list"

interfaces [
-> { Decidim::Core::HashtagInterface }

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

Will this interface be used by other types? If it won't, then I suggest putting the fields definition directly to the type to reduce complexity.

t.references :decidim_hashtag, index: true
t.references :decidim_hashtaggable, polymorphic: true, index: { name: "idx_hashtaggins_on_hashtaggable_type_and_hashtaggable_id" }
end
add_index :decidim_hashtaggings, %w(decidim_hashtaggable_id decidim_hashtaggable_type),

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

Isn't this a duplicate from the index in line 7?

name "HashtagInterface"
description "A hashtag"

field :name, !types.String, "The hashtag's name"

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

I suggest moving this to the HashtagType and deleting this file.

# hashtag with a global id.
#
# @return [String] the content with the hashtags replaced by a global id

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

Remove extra white line


context "when hashtagging a non created hashtag" do
let(:hashtag3) { create(:hashtag, organization: organization) }
let(:content) { "This text hashtagiing a non created hashtag: ##{hashtag3.name}" }

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

hashtag3 does exist in the DB, so this test is wrong.

title: @form.title,
description: @form.description,
# title: @form.title,
title: parser.parse(@form.title),

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

Can titles have hashtags? Won't this be weird when rendering?

This comment has been minimized.

@isaacmg410

isaacmg410 Aug 20, 2018
Author Contributor

a long time ago, @xabier said that fields were title and body...
Maybe @decidim/product can clarify

# parsed_title = Decidim::ContentProcessor.parse(form.title, current_organization: current_organization).rewrite
# parsed_body = Decidim::ContentProcessor.parse(form.body, current_organization: current_organization).rewrite

parsed_title = Decidim::ContentProcessor.parse_with_processor(:hashtag, form.title, current_organization: current_organization).rewrite

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

Move these 2 lines inside the attributes method and you won't need to pass those as params

renderer.render.html_safe
end

# This methods return the plain title

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

Wrong docs

@@ -6,7 +6,7 @@
<%= proposal.id %><br />
</td>
<td>
<%= proposal.title %><br />

This comment has been minimized.

@mrcasals

mrcasals Aug 6, 2018
Contributor

Missing <br />? Don't know if it's intended though.

}).fail(function() {
cb([])
}).always(() => {
// This function runs Tribute every single time you type somthing

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

s/somthing/something 馃

if ($(this.current.element).hasClass("editor-container")) {
let quill = this.current.element.__quill;
quill.insertText(cursor - 1, `#${item.original.name} `, Quill.sources.API);
// cursor position + nickname length + "@" sign + space

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

Nickname or hashtag?

}
});

function remoteSearch(text, cb) {

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

Duplicated function?

end

def hashtags
Decidim::Hashtag.where(organization: @organization).where("name like ?", "#{@term}%")

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

Why use like?

This comment has been minimized.

@isaacmg410

isaacmg410 Aug 20, 2018
Author Contributor

to collect hashtags with the names thah begin with 'bla' for example

This comment has been minimized.

@oriolgual

oriolgual Aug 27, 2018
Member

Then don't we need something like "name like ?%"?

This comment has been minimized.

@oriolgual

oriolgual Aug 27, 2018
Member

Oh, I didn't see the % at the end, sorry.

This comment has been minimized.

@isaacmg410

isaacmg410 Aug 27, 2018
Author Contributor

Don't worry 馃槃

name "HashtagInterface"
description "A hashtag"

field :name, !types.String, "The hashtag's name"

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

Since this interface won't be used anywhere else, you can drop it and move the field to the type

#
# @see BaseRenderer Examples of how to use a content renderer
class HashtagRenderer < BaseRenderer
# Matches a global id representing a Decidim::User

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

s/User/Hashtag

HASHTAG_REGEX = /(^|\s)#([a-zA-Z0-9]\w*)/i

def hashtaggables
decidim_hashtaggings.includes(:decidim_hashtaggable).collect(&:decidim_hashtaggable)

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

Should we memoize this?

decidim_hashtaggings.includes(:decidim_hashtaggable).collect(&:decidim_hashtaggable)
end

def hashtagged_types

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

These methods aren't used

@@ -40,17 +40,23 @@ def call
attr_reader :form, :proposal, :attachment

def create_proposal
# parsed_title = Decidim::ContentProcessor.parse(form.title, current_organization: current_organization).rewrite

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

Remove this?

# parsed_title = Decidim::ContentProcessor.parse(form.title, current_organization: current_organization).rewrite
# parsed_body = Decidim::ContentProcessor.parse(form.body, current_organization: current_organization).rewrite

parsed_title = Decidim::ContentProcessor.parse_with_processor(:hashtag, form.title, current_organization: current_organization).rewrite

This comment has been minimized.

@oriolgual

oriolgual Aug 6, 2018
Member

Isn't a bit weird to allow hashtags at titles?

This comment has been minimized.

@isaacmg410

isaacmg410 Aug 20, 2018
Author Contributor

@decidim/product clarify this please.

This comment has been minimized.

@carolromero

carolromero Aug 20, 2018
Member

@isaacmg410 @oriolgual I have checked the conversation in the original issue and we agreed to ignore the hashtags in the title: #2458 (comment)

This comment has been minimized.

@isaacmg410

isaacmg410 Aug 20, 2018
Author Contributor

so that is what we are doing.
Show the hashtag on title ignoring the link 馃槃

@isaacmg410 isaacmg410 changed the title [WIP] Hashtags Feature applied to Proposals Hashtags Feature applied to Proposals Aug 27, 2018
Isaac Massot added 5 commits Aug 27, 2018
@@ -17,6 +17,7 @@ class Meeting < Meetings::ApplicationRecord
include Decidim::Searchable
include Decidim::Traceable
include Decidim::Loggable
include Decidim::Hashtaggable

This comment has been minimized.

@oriolgual

oriolgual Aug 27, 2018
Member

Shouldn't this be in another PR?

This comment has been minimized.

@isaacmg410

isaacmg410 Aug 27, 2018
Author Contributor

You are right!

Isaac Massot added 6 commits Aug 27, 2018
isaacmg410
@isaacmg410 isaacmg410 added in-review and removed status: WIP labels Aug 28, 2018
@isaacmg410
Copy link
Contributor Author

@isaacmg410 isaacmg410 commented Aug 29, 2018

@oriolgual requested changes applied, ready to review and merge

@ghost ghost assigned oriolgual Aug 29, 2018
@ghost ghost added the status: WIP label Aug 29, 2018
@mrcasals mrcasals removed the status: WIP label Aug 30, 2018
@mrcasals mrcasals merged commit 006a084 into master Aug 30, 2018
27 of 28 checks passed
27 of 28 checks passed
WIP work in progress
Details
ci/circleci: accountability Your tests passed on CircleCI!
Details
ci/circleci: admin Your tests passed on CircleCI!
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: assemblies Your tests passed on CircleCI!
Details
ci/circleci: blogs Your tests passed on CircleCI!
Details
ci/circleci: budgets Your tests passed on CircleCI!
Details
ci/circleci: build_design_app Your tests passed on CircleCI!
Details
ci/circleci: build_test_app Your tests passed on CircleCI!
Details
ci/circleci: comments Your tests passed on CircleCI!
Details
ci/circleci: consultations Your tests passed on CircleCI!
Details
ci/circleci: core Your tests passed on CircleCI!
Details
ci/circleci: debates Your tests passed on CircleCI!
Details
ci/circleci: generators Your tests passed on CircleCI!
Details
ci/circleci: initiatives Your tests passed on CircleCI!
Details
ci/circleci: main Your tests passed on CircleCI!
Details
ci/circleci: meetings Your tests passed on CircleCI!
Details
ci/circleci: pages Your tests passed on CircleCI!
Details
ci/circleci: participatory_processes Your tests passed on CircleCI!
Details
ci/circleci: proposals Your tests passed on CircleCI!
Details
ci/circleci: sortitions Your tests passed on CircleCI!
Details
ci/circleci: surveys Your tests passed on CircleCI!
Details
ci/circleci: system Your tests passed on CircleCI!
Details
ci/circleci: upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: verifications Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 95% (80% threshold)
Details
codeclimate/total-coverage 98% (0.0% change)
Details
@mrcasals mrcasals deleted the 2458_hashtags branch Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants