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

Adds Tagging module #7347

Merged
merged 2 commits into from May 21, 2016

Conversation

Projects
None yet
6 participants
@vishalzambre
Contributor

vishalzambre commented May 11, 2016

PR for #7343
Added acts_as_taggable to spree.
Integrated with product for now.

'use strict';
function formatTag(tag) {
return Select2.util.escapeMarkup(tag.name);

This comment has been minimized.

@houndci-bot

houndci-bot May 11, 2016

'Select2' is not defined.

@houndci-bot

houndci-bot May 11, 2016

'Select2' is not defined.

data: function (term) {
return {
q: term,
token: Spree.api_key

This comment has been minimized.

@houndci-bot

houndci-bot May 11, 2016

Identifier 'api_key' is not in camel case.
'Spree' is not defined.

@houndci-bot

houndci-bot May 11, 2016

Identifier 'api_key' is not in camel case.
'Spree' is not defined.

callback(data);
},
ajax: {
url: Spree.routes.tags_search,

This comment has been minimized.

@houndci-bot

houndci-bot May 11, 2016

Identifier 'tags_search' is not in camel case.
'Spree' is not defined.

@houndci-bot

houndci-bot May 11, 2016

Identifier 'tags_search' is not in camel case.
'Spree' is not defined.

}
this.select2({
placeholder: Spree.translations.tags_placeholder,

This comment has been minimized.

@houndci-bot

houndci-bot May 11, 2016

Identifier 'tags_placeholder' is not in camel case.
'Spree' is not defined.

@houndci-bot

houndci-bot May 11, 2016

Identifier 'tags_placeholder' is not in camel case.
'Spree' is not defined.

@@ -62,6 +63,7 @@ Spree.routes.stock_locations_api = Spree.pathFor('api/v1/stock_locations')
Spree.routes.taxon_products_api = Spree.pathFor('api/v1/taxons/products')
Spree.routes.taxons_search = Spree.pathFor('api/v1/taxons')
Spree.routes.user_search = Spree.adminPathFor('search/users')
Spree.routes.tags_search = Spree.adminPathFor('search/tags')

This comment has been minimized.

@houndci-bot

houndci-bot May 11, 2016

Identifier 'tags_search' is not in camel case.
Missing semicolon.

@houndci-bot

houndci-bot May 11, 2016

Identifier 'tags_search' is not in camel case.
Missing semicolon.

@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre May 12, 2016

Contributor

@damianlegawiec Shall I resolve these comments for double-quoted for strings because in current code most of places have mixed format and as per Rubocop don't use double quote unless you are not interpolating string.

Contributor

vishalzambre commented May 12, 2016

@damianlegawiec Shall I resolve these comments for double-quoted for strings because in current code most of places have mixed format and as per Rubocop don't use double quote unless you are not interpolating string.

@damianlegawiec

This comment has been minimized.

Show comment
Hide comment
@damianlegawiec

damianlegawiec May 13, 2016

Member

@vishalzambre - great work - my only concern is that you've added a migration for tags without the spree prefix

Member

damianlegawiec commented May 13, 2016

@vishalzambre - great work - my only concern is that you've added a migration for tags without the spree prefix

@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre May 13, 2016

Contributor

@damianlegawiec Its because of I used acts_as_taggable gem and they have table names as I added in migration, but for user purpose I added Spree::Tag model which is inherited from gem Tag model.

I think It will not affect, If we need spree prefix then I need to override models from gem, Let me know, If it requires I'll do that.

Contributor

vishalzambre commented May 13, 2016

@damianlegawiec Its because of I used acts_as_taggable gem and they have table names as I added in migration, but for user purpose I added Spree::Tag model which is inherited from gem Tag model.

I think It will not affect, If we need spree prefix then I need to override models from gem, Let me know, If it requires I'll do that.

@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre May 17, 2016

Contributor

@damianlegawiec I think it will not affect anything to have migrations without prefix spree.

Contributor

vishalzambre commented May 17, 2016

@damianlegawiec I think it will not affect anything to have migrations without prefix spree.

@damianlegawiec

This comment has been minimized.

Show comment
Hide comment
@damianlegawiec

damianlegawiec May 17, 2016

Member

@vishalzambr this will result in a lot of trouble for anybody who has already acts_as_taggable in their projects, that's why the prefix is a must IMO

@priyank-gupta your thoughts?

Member

damianlegawiec commented May 17, 2016

@vishalzambr this will result in a lot of trouble for anybody who has already acts_as_taggable in their projects, that's why the prefix is a must IMO

@priyank-gupta your thoughts?

@priyank-gupta

This comment has been minimized.

Show comment
Hide comment
@priyank-gupta

priyank-gupta May 17, 2016

Contributor

@damianlegawiec Agreed. This also helps in scoping migrations if already present with same name and easily identifiable as which gem/extension added that migration.

Contributor

priyank-gupta commented May 17, 2016

@damianlegawiec Agreed. This also helps in scoping migrations if already present with same name and easily identifiable as which gem/extension added that migration.

@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre May 17, 2016

Contributor

@damianlegawiec @priyank-gupta I'll modify migrations and will add them under spree namespce.

Contributor

vishalzambre commented May 17, 2016

@damianlegawiec @priyank-gupta I'll modify migrations and will add them under spree namespce.

class ChangeCollationForTagNames < ActiveRecord::Migration
def up
if ActsAsTaggableOn::Utils.using_mysql?
execute("ALTER TABLE spree_tags MODIFY name varchar(255) CHARACTER SET utf8 COLLATE utf8_bin;")

This comment has been minimized.

@houndci-bot

houndci-bot May 17, 2016

Line is too long. [101/80]

@houndci-bot

houndci-bot May 17, 2016

Line is too long. [101/80]

This comment has been minimized.

@houndci-bot

houndci-bot May 17, 2016

Line is too long. [101/80]

@houndci-bot

houndci-bot May 17, 2016

Line is too long. [101/80]

This comment has been minimized.

@krzysiek1507
@krzysiek1507

This comment has been minimized.

@vishalzambre

vishalzambre May 27, 2016

Contributor

@krzysiek1507 If you checked log from travis-ci its executing execute("ALTER TABLE tags MODIFY name varchar(255) CHARACTER SET utf8 COLLATE utf8_bin;") command which is not present in this migration, Yes but this is in gem acts_as_taggable.

@vishalzambre

vishalzambre May 27, 2016

Contributor

@krzysiek1507 If you checked log from travis-ci its executing execute("ALTER TABLE tags MODIFY name varchar(255) CHARACTER SET utf8 COLLATE utf8_bin;") command which is not present in this migration, Yes but this is in gem acts_as_taggable.

This comment has been minimized.

@vishalzambre

vishalzambre May 27, 2016

Contributor

@krzysiek1507 I'm not getting why it is executing migrations from gem and if it is then it should have run all migration from gem

@vishalzambre

vishalzambre May 27, 2016

Contributor

@krzysiek1507 I'm not getting why it is executing migrations from gem and if it is then it should have run all migration from gem

This comment has been minimized.

@krzysiek1507

krzysiek1507 May 27, 2016

Contributor

@vishalzambre Can you check how customize this gem?

@krzysiek1507

krzysiek1507 May 27, 2016

Contributor

@vishalzambre Can you check how customize this gem?

This comment has been minimized.

@vishalzambre

vishalzambre May 27, 2016

Contributor

@krzysiek1507 Yes sure I'll check

@vishalzambre

vishalzambre May 27, 2016

Contributor

@krzysiek1507 Yes sure I'll check

vishalzambre added some commits May 11, 2016

@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre May 17, 2016

Contributor

@damianlegawiec @priyank-gupta Added spree prefix to migrations

Contributor

vishalzambre commented May 17, 2016

@damianlegawiec @priyank-gupta Added spree prefix to migrations

@damianlegawiec damianlegawiec added this to the v3.2.x milestone May 18, 2016

@damianlegawiec

This comment has been minimized.

Show comment
Hide comment
@damianlegawiec

damianlegawiec May 21, 2016

Member

Nice work @vishalzambre 👍

Member

damianlegawiec commented May 21, 2016

Nice work @vishalzambre 👍

@damianlegawiec damianlegawiec merged commit 1c9b5a6 into spree:master May 21, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
hound 9 violations found.
@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre May 21, 2016

Contributor

@damianlegawiec @priyank-gupta Hey, I'd be happy to solve bug or add features in spree if you have anything in bucket please assign me.

Contributor

vishalzambre commented May 21, 2016

@damianlegawiec @priyank-gupta Hey, I'd be happy to solve bug or add features in spree if you have anything in bucket please assign me.

@tommiller1

This comment has been minimized.

Show comment
Hide comment
@tommiller1

tommiller1 May 22, 2016

Contributor

NOTE: Migration 20160511071954_acts_as_taggable_on_migration.rb from spree has been skipped. Migration with the same name already exists.
NOTE: Migration 20160511072249_change_collation_for_tag_names.rb from spree has been skipped.
Migration with the same name already exists.

Please change the file names too.

Contributor

tommiller1 commented May 22, 2016

NOTE: Migration 20160511071954_acts_as_taggable_on_migration.rb from spree has been skipped. Migration with the same name already exists.
NOTE: Migration 20160511072249_change_collation_for_tag_names.rb from spree has been skipped.
Migration with the same name already exists.

Please change the file names too.

@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre May 23, 2016

Contributor

@tommiller1 I checked in spree core db/migrations, I'm not getting any files having same name, and If that was the case then spec should be failed, Its passing spec

Contributor

vishalzambre commented May 23, 2016

@tommiller1 I checked in spree core db/migrations, I'm not getting any files having same name, and If that was the case then spec should be failed, Its passing spec

@tommiller1

This comment has been minimized.

Show comment
Hide comment
@tommiller1

tommiller1 May 23, 2016

Contributor

It clashes for people who already have acts_as_taggable installed.

Contributor

tommiller1 commented May 23, 2016

It clashes for people who already have acts_as_taggable installed.

@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre May 23, 2016

Contributor

@tommiller1 Right, make sense, raised PR #7369

Contributor

vishalzambre commented May 23, 2016

@tommiller1 Right, make sense, raised PR #7369

@vishalzambre vishalzambre deleted the vishalzambre:tagging branch May 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment