Change gemfile for travis pipeline by Pedro-Pezoa · Pull Request #24 · trustvox/sales_up

Pedro-Pezoa · 2019-03-08T14:46:23Z

Travis alerted that the build of the application broke because of some mistakes inside gemfile like double instances of gems. The file were changed and submited to a PR evaluation.

All the files listed were changed according with the rubocop code evaluation, mostly for methods with many code lines and process time.

For Travis build changes, the test regarding user model were updated to use the new area/sub_area attributes instead of the priority type.

All the files listed were changed according with the rubocop code evaluation automaticaly.

The user file were modified to update the priority attribute with the new area/sub_area.

For refactor purpose about CodeClimate evaluation, the add_filter_elements method inside overview_points were changed to use only 4 parameters, not including now the index variable.

brunoocasali

@Pedro-Pezoa valeuu man pelo fix! Fiz algumas sugestões pra ti, confere elas aí!

brunoocasali · 2019-03-12T17:01:35Z

app/controllers/concerns/database_searchs.rb

  end

-  def month_finished?(day)
+  def month_finished?(_day)


Se o day não é usado, ele nem precisaria ser passado aqui não?

brunoocasali · 2019-03-12T17:09:47Z

app/controllers/concerns/overview_points.rb

@@ -98,8 +97,7 @@ def am_filter(user_id, index, first, type)
    when 'CS'
      [index, fetch_contract_sum_with_ids(user_id, first.id)]
    when 'CP'


Quase ninguém mais usa case/when em ruby, até acho estranho o rubocop não ter se ofendido com isso.
https://www.rubyguides.com/2015/10/ruby-case/ artigo maneiro sobre quando usar ou não.

Uma ideia pra melhorar isso seria:

filter = case init_filter(type) when 'CS' fetch_contract_sum_with_ids(user_id, first.id) when 'CP' (calculate_contract_percentage * 100).round(1) when 'CC' fetch_contracts_by_user_report_id(user_id, first.id) when 'FC' fetch_deal_sum_with_ids(user_id, first.id) end [index, filter]

Aliás, se tu perceber o index não atua dentro do método e sim, ele simplesmente entra e sai sem participar de nenhuma ação. Maybe ele nem precisaria entrar nesse método ;)
E first talvez não seja um nome de argumento tão bom, ele não me dá muito contexto quando vejo só o método ;D

brunoocasali · 2019-03-12T17:10:45Z

app/controllers/concerns/overview_points.rb

+      index += 1
    end
+
+    list << verify_filter_option(user.id, index, first, type)


Se no método de verificação não precisasse passar o index esse método ficaria:

list << [index, verify_filter_option(user.id, first, type)]

brunoocasali · 2019-03-12T17:15:04Z

app/controllers/concerns/report_data_points.rb

@@ -82,11 +82,13 @@ def int_to_weekday(date)
    return 'Tuesday' if date.tuesday?


Esse método pode ser melhorado, usando um hash, ou procurando outras alternativas no ActiveSupport ou no próprio ruby...

date.strftime('%A') => 'Monday'

No caso se esse for o objetivo do método mesmo, ele nem precisaria existir ;)

https://github.com/trustvox/sales_up/pull/24/files#diff-809770723468cbce0793eda96affafcfR76

Tu poderia apenas usar:

(@first_date..@last_date).collect { |date| date.strftime('%A') }

E além disso, existe uma gem chamada https://github.com/bokmann/business_time que pode ser realmente útil se você precisa fazer mais do que apenas saber quais dias são dias úteis ou não!

brunoocasali · 2019-03-12T17:27:30Z

app/controllers/user_controller.rb

-    contract.day = Time.current.day
-    contract.report_id = fetch_last_report('am').id
+  def create_contract
+    create_contract_helper


Helper no rails tem outra finalidade, ele geralmente lida com recursos de view ex:

form_tag e text_input, t, l por exemplo são helpers. Seria bacana você dar um novo nome para estes métodos.

brunoocasali · 2019-03-12T17:36:38Z

app/helpers/integration_helper.rb

@@ -0,0 +1,89 @@
+module IntegrationHelper


Evite usar nomes genéricos demais, pois isso dificulta o entendimento na medida que o sistema cresce!

brunoocasali · 2019-03-12T17:37:23Z

app/helpers/integration_helper.rb

+  end
+
+  def fetch_contract_ids
+    @contract.report_id = fetch_last_report('am').id


vejo essas siglas por todo o canto, já pensou se fosse necessário alterá-la?

brunoocasali · 2019-03-12T17:38:36Z

app/models/user.rb

    area == 'sales'
  end

+  def admin?


Bora mover esses métodos de uma linha para um concern?

brunoocasali · 2019-03-12T17:40:05Z

spec/models/user_spec.rb

  context 'instance methods' do
-    describe '#regular?' do
-      let(:priority) { 1 }
+    describe '#overall_manager?' do


Você concorda que faltam mais testes por aqui?

Destes métodos de uma linha no caso ;)

brunoocasali · 2019-03-12T17:40:33Z

config/zapier_ruby.example.rb

@@ -1,3 +1,3 @@
 ZapierRuby.configure do |c|
-  c.web_hooks = {example_zap: "xxxxxxx/yyyyyy" }
+  c.web_hooks = { example_zap: 'xxxxxxx/yyyyyy' }


Esse arquivo está sendo utilizado?

brunoocasali · 2019-03-12T17:43:32Z

@Pedro-Pezoa só acho que tu misturou duas coisas nesse PR, a de alteração da prioridade dos usuários e a correção do Travis!

Na próxima a gente divide cada um no seu quadrado! :D

Change gemfile for travis pipeline

a5c35ac

Travis alerted that the build of the application broke because of some mistakes inside gemfile like double instances of gems. The file were changed and submited to a PR evaluation.

Pedro-Pezoa self-assigned this Mar 8, 2019

Pedro-Pezoa requested a review from brunoocasali March 8, 2019 14:46

Pedro-Pezoa added 5 commits March 8, 2019 12:56

Rubocop refactor manualy

cbcf0f1

All the files listed were changed according with the rubocop code evaluation, mostly for methods with many code lines and process time.

Update spec tests with the new area/sub_area attribute

fa16d85

For Travis build changes, the test regarding user model were updated to use the new area/sub_area attributes instead of the priority type.

Rubocop refactor automatically

828ab11

All the files listed were changed according with the rubocop code evaluation automaticaly.

Change priority attribute in user Factory

7c2f6dc

The user file were modified to update the priority attribute with the new area/sub_area.

Change add filter parameters

fe7853c

For refactor purpose about CodeClimate evaluation, the add_filter_elements method inside overview_points were changed to use only 4 parameters, not including now the index variable.

brunoocasali suggested changes Mar 12, 2019

View reviewed changes

brunoocasali mentioned this pull request Mar 12, 2019

Spec test contract #25

Open

		@@ -82,11 +82,13 @@ def int_to_weekday(date)
		return 'Tuesday' if date.tuesday?

Conversation

Pedro-Pezoa commented Mar 8, 2019

Uh oh!

brunoocasali left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunoocasali commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

brunoocasali left a comment •

edited

Loading