Add skills to the user profile #7

H889761123 · 2017-06-15T20:43:25Z

The pull request is based on #5

though we still didn't agree 100% whether we should use tiles or tags (with auto-complete), I really think it is better to use the tags concept (and we can discuss that in the comments if anyone disagree..)

This feature will provide the user with the ability to select multiple skills and add them to his/her profile.

There will be an auto-complete list for skills, however a user can still add his/her own skills (and the new custom skill name will be available in the tags table but it won't appear in the general auto-complete list for all users until is_approved is set to true in the tags table [by an admin])

I believe having diversity of tags is better than having just few general ones and this should be encouraged so for example instead of the user types "deploying" only, it is better to also add (docker,AWS,etc..) <= that will make the platform more powerful and help other programmers to easily find other programmers with a specific skill [which I personally consider a huge added value for programmers to use the platform - especially to find collaborators]

that's why as a start I retrieved a list of programming languages, and I added them to seeds.rb (<= and of course we need to update this list with other skills, but consider it a starting point)

and on the profile page, each skill should link to a search result (currently it links to '#' but this can be updated later).

Here are some screenshots:

1- before triggering the auto-complete:

2- auto-complete:

3- profile with linkable skills:

turbolinks is affecting JQuery code in certain situations which causes some features not to work as expected

Link- · 2017-06-15T20:56:18Z

That is one juicy PR! Nice! 👍 👍

Link- · 2017-06-15T20:57:56Z

app/assets/javascripts/custom.js

@@ -7,4 +7,13 @@ $("document").ready(function() {
        return false;
    }
   })
+
+
+   $(".skills-input").select2({


Am I looking at a spacing issue here?

Yeah there are some extra spaces, will remove them

Fixed: adebefb

constantine-nikolaou · 2017-06-15T21:57:57Z

Testing this one @HassanKanj

constantine-nikolaou

That's a very impressive Merge Request @HassanKanj, i've left a couple of comments to make it more solid. Let me know once you get the chance to go over them.

constantine-nikolaou · 2017-06-15T22:04:16Z

app/views/profiles/_form.html.erb

+<label>Skills</label>
+<select id="skill_list" name="skill_list[]" class="skills-input" class="js-example-basic-single" multiple="multiple">
+
+  User.where(weekly_subscriber: true).find_each do |user|


Part of MVC best practices, queries and interactions with ORM always happen in the controller, not in the views. This needs moving to the controller.

sure will work on that, btw the weekly_subscriber part isn't actually part of the code, I pasted it by mistake, I just used it as a reference

Removed: 004df70

constantine-nikolaou · 2017-06-15T22:04:39Z

app/views/profiles/_form.html.erb

+  end
+
+
+  <% Tag.where(is_approved: true).find_each do |skill| %>


This also needs moving to the controller.

Fixed, let me know if this is acceptable: 94cc105 (Note: the commit also includes bug fix)

constantine-nikolaou · 2017-06-15T22:06:56Z

app/views/profiles/_form.html.erb

+</select>
+<span class="help-block skills-help">Note: if a skill doesn't exist in the list, manually type it and press enter.</span>
+
+<script type="text/javascript">


JS tags are included at the end of the view if they're needed to be included once in a very specific way.
I suggest we move this one to a file under app/assets/javascripts/ and add it to the list of JS files that are loaded under assets.rb

It is cleaner to move it, but there are 2 problems:

1- This code is only needed in this specific form
2- The script actually depends on this ruby variable: @profile.skill_list

So maybe I can move it to the end of the file as you suggested? unless you have another solution in mind

constantine-nikolaou · 2017-06-15T22:07:40Z

db/migrate/20170613071825_change_collation_for_tag_names.acts_as_taggable_on_engine.rb

+# This migration comes from acts_as_taggable_on_engine (originally 5)
+# This migration is added to circumvent issue #623 and have special characters
+# work properly
+class ChangeCollationForTagNames < ActiveRecord::Migration


This migration file can go since we are not using MySQL DB

Removed: 0bcf21c

constantine-nikolaou · 2017-06-15T22:08:23Z

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema.define(version: 20170319214240) do
+ActiveRecord::Schema.define(version: 20170613071826) do


Migration files can be merged all into a final one instead of having many files.

any specific way to do that?
what I tried so far is to create a single migration file and I combined up, down and change from the separate files together and I also deleted the old migration files.. but it doesn't seem it is working!

here's the final file (20170616175054_acts_as_taggable_on_migration.rb):

# This migration comes from acts_as_taggable_on_engine (Note: they were manually combined in a single file) class ActsAsTaggableOnMigration < ActiveRecord::Migration[5.0] def self.up create_table :tags do |t| t.string :name t.boolean :is_approved, default: false end create_table :taggings do |t| t.references :tag # You should make sure that the column created is # long enough to store the required class names. t.references :taggable, polymorphic: true t.references :tagger, polymorphic: true # Limit is created to prevent MySQL error on index # length for MyISAM table type: http://bit.ly/vgW2Ql t.string :context, limit: 128 t.datetime :created_at end add_index :taggings, :tag_id add_index :taggings, [:taggable_id, :taggable_type, :context] add_index :tags, :name, unique: true remove_index :taggings, :tag_id if index_exists?(:taggings, :tag_id) remove_index :taggings, [:taggable_id, :taggable_type, :context] add_index :taggings, [:tag_id, :taggable_id, :taggable_type, :context, :tagger_id, :tagger_type], unique: true, name: 'taggings_idx' add_column :tags, :taggings_count, :integer, default: 0 ActsAsTaggableOn::Tag.reset_column_information ActsAsTaggableOn::Tag.find_each do |tag| ActsAsTaggableOn::Tag.reset_counters(tag.id, :taggings) end add_index :taggings, [:taggable_id, :taggable_type, :context] end # up def self.down drop_table :taggings drop_table :tags remove_index :tags, :name remove_index :taggings, name: 'taggings_idx' add_index :taggings, :tag_id unless index_exists?(:taggings, :tag_id) add_index :taggings, [:taggable_id, :taggable_type, :context] remove_column :tags, :taggings_count remove_index :taggings, [:taggable_id, :taggable_type, :context] end # down def change add_index :taggings, :tag_id add_index :taggings, :taggable_id add_index :taggings, :taggable_type add_index :taggings, :tagger_id add_index :taggings, :context add_index :taggings, [:tagger_id, :tagger_type] add_index :taggings, [:taggable_id, :taggable_type, :tagger_id, :context], name: 'taggings_idy' end # change end # class

Yeah, each table should have one single migration file.

Methods up/down are actually combined in one method change.

up/down are called the old migration methods, you can achieve the same intended behaviour using change http://guides.rubyonrails.org/active_record_migrations.html#using-the-change-method

I was actually using the multiple migration files to generate the files! but I should've used the schema.db instead.
Done: 2d82bd3

constantine-nikolaou · 2017-06-15T22:09:42Z

db/seeds.rb

+
+
+# add skills
+Tag.create(name: "A# .NET", is_approved: true)


Super expensive seeds.rb document.
Try instead to have all values in an array, then loop through the array and create all of the values in one go.

No loop required, .create supports bulk creation: http://api.rubyonrails.org/classes/ActiveRecord/Persistence/ClassMethods.html#method-i-create

That's cool! will try it without looping then

Done: 0496ff5

constantine-nikolaou · 2017-06-16T20:33:00Z

@HassanKanj the Merge Request has conflicting files, you need to pull the changes, resolve the conflict please and update the Merge Request branch.

H889761123 · 2017-06-17T10:07:29Z

@cnicolaou I pulled the changes and resolved the conflicts: 6e2e2ba

constantine-nikolaou · 2017-06-27T21:32:18Z

@HassanKanj i think you can pull and update this PR now

H889761123 · 2017-07-20T00:22:02Z

I will close this pull request and replace it with another similar one

HassanKanj added 7 commits June 13, 2017 10:01

Update development.yml (to avoid letter opener exceptions)

61aa0f7

Add 'acts-as-taggable-on' gem

5a5e9e6

Add tags (skills) migrations

0875056

Remove turbolinks and add select2

d0a484c

turbolinks is affecting JQuery code in certain situations which causes some features not to work as expected

Add select2-rails gem

81ce71f

Modify/View user skills

5254556

Merge branch 'hkanj-skills-feature'

d725b21

Link- reviewed Jun 15, 2017

View reviewed changes

constantine-nikolaou reviewed Jun 15, 2017

View reviewed changes

HassanKanj added 5 commits June 16, 2017 12:11

Remove extra spaces

adebefb

Remove an unrelated code snippet that I kept by mistake

004df70

Add user skills + approved skills to the autocomplete list

94cc105

Remove a migration file (since we are not using MySQL)

0bcf21c

Add the skills to an array

0496ff5

HassanKanj added 7 commits June 17, 2017 12:06

Add additional skills

322c3a6

Replace multiple migration files by 2 files only

2d82bd3

Update Gemfile.lock

f3d0e5c

Modify .gitignore

9787998

Resolving conflicts

6e2e2ba

Clean extra spaces

e115445

Add schema information to Tag

29bcd29

H889761123 closed this Jul 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add skills to the user profile #7

Add skills to the user profile #7

H889761123 commented Jun 15, 2017

Link- commented Jun 15, 2017

Link- Jun 15, 2017 •

edited

Loading

H889761123 Jun 15, 2017

H889761123 Jun 16, 2017

constantine-nikolaou commented Jun 15, 2017

constantine-nikolaou left a comment

constantine-nikolaou Jun 15, 2017

H889761123 Jun 16, 2017

H889761123 Jun 16, 2017

constantine-nikolaou Jun 15, 2017

H889761123 Jun 16, 2017

constantine-nikolaou Jun 15, 2017

H889761123 Jun 16, 2017

constantine-nikolaou Jun 15, 2017

H889761123 Jun 16, 2017

constantine-nikolaou Jun 15, 2017

H889761123 Jun 16, 2017

constantine-nikolaou Jun 16, 2017

H889761123 Jun 17, 2017

constantine-nikolaou Jun 15, 2017

omaraboumrad Jun 16, 2017

H889761123 Jun 16, 2017

H889761123 Jun 16, 2017

constantine-nikolaou commented Jun 16, 2017

H889761123 commented Jun 17, 2017

constantine-nikolaou commented Jun 27, 2017

H889761123 commented Jul 20, 2017

Add skills to the user profile #7

Add skills to the user profile #7

Conversation

H889761123 commented Jun 15, 2017

Link- commented Jun 15, 2017

Link- Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

constantine-nikolaou commented Jun 15, 2017

constantine-nikolaou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

constantine-nikolaou commented Jun 16, 2017

H889761123 commented Jun 17, 2017

constantine-nikolaou commented Jun 27, 2017

H889761123 commented Jul 20, 2017

Link- Jun 15, 2017 •

edited

Loading