Ticket_id #52283 - Alinhamento com o Fork da Hotsoft #3

hotsoft-desenv2 · 2016-11-23T19:27:53Z

Se possível aceitem este pull request relativo ao alinhamento com o Fork da Hotsoft.

…mentos.

…amework

acras · 2016-11-24T10:58:23Z

Obrigado pelo pull request. Eu vou comentar lá, mas para registro vou comentar o mesmo por aqui também.

Para que os commits fiquem mais limpos, por favor evite deixar o código anterior comentado, como aqui por exemplo: 2bd352f
O git se encarrega de mostrar como o código era e como ele ficou.

Outro exemplo de "limpeza" seria neste commit: 64afab2

Aqui o arquivo ac_filters_utils.rb tem código anterior comentado, referência ao ticket resolvido no comentário do fonte e ainda uma alteração que não tem relação com o que o commit resolve.

Como boa prática os commits devem fazer somente o que se propõe e também o que foi alterado deve ser alterado e não comentado.

Peço por favor que ajuste o pull request neste sentido e reenvie para eu analisar.

…. Removido Ticket removido do fonte.

hotsoft-desenv2 · 2016-11-24T12:17:06Z

Bom dia @acras,

realizei a limpeza conforme solicitado e seguirei este padrão.
Segue o link com as alterações de limpeza: a5ff925
Segue estão este pull request para uma nova análise.

hotsoft-desenv2 · 2016-11-24T12:37:50Z

repo/reports/simple_listing.rb

      @data_end
    end

-    def new_page(print_titles = true)


Trouxe manualmente dos commits do nosso form.
Evita problemas com alguns relatórios.

hotsoft-desenv2 · 2016-11-24T12:38:25Z

repo/reports/listing.rb

  class Listing < Report

-    #alias :super_new_page :new_page
+    alias :super_new_page :new_page


Supre uso do super_new_page em alguns relatórios.

Não entendi o objetivo desta alteração.

Segue o Ticket: http://192.168.200.44:3000/issues/22060

É utilizado em alguns de nossos relatórios.
Alteração realizada para evitar problema com os relatórios que utilizam este alias.
Conforme o Ticket citado acima.

hotsoft-desenv2 · 2016-11-24T12:39:10Z

prawn_report.gemspec

-  s.email = %q{ricardo@acras.com.br julianoch@gmail.com egon@acras.com.br viniciuscb@gmail.com}
+  s.version = "1.9.21"
+  s.date = %q{2016-11-23}
+  s.authors = ["Ricardo Acras", "Egon Hilgenstieler", "Juliano Andrade", "Vinicius Cubas Brand", "Wellington Torrejais da Silva"]


Array de emails não estava separado por ","

Agora segue conforme a documentação.

hotsoft-desenv2 · 2016-11-24T12:40:28Z

lib/report_helpers.rb

      if !value.nil? && value != ''
        if (formatter == :currency)
-          value = value.round(2)
+          value = value.round(2) rescue sprintf('%.2f', value).to_f


Gera problema com versões do framework mais antigas como no nossa caso.
Um simples rescue resolve o problema mantendo compatibilidade com Ruby antigo.

hotsoft-desenv2 · 2016-11-24T12:41:53Z

lib/report.rb

    def run_totals(data_row)
      @running_totals.each do |rt|
        vl = get_raw_field_value(data_row, rt)
+        vl = (vl.is_a? (String) ? vl.to_f : vl)


Gerava problemas com alguns tipos de dados do Mysql.
De acordo com a query e o tipo retornado.

Este ajuste evitou muitos problemas com médias e suas totalizações.

hotsoft-desenv2 · 2016-11-24T12:53:36Z

lib/ac_filters_utils.rb

      sql_to_execute = ActiveRecord::Base.send(:sanitize_sql_array, conditions)
      r = []
-      ActiveRecord::Base.connection.instance_exec(sql_to_execute).each(:as => :hash) {|i| r << i}
+      ActiveRecord::Base.connection.instance_exec(sql_to_execute).each(as :hash) {|i| r << i}


Alteração sem relação direta com o commit portada do nosso fork.

O que faz esta alteração?

Só troca de sintaxe mesmo. Posso remover esta alteração.
Trouxe dos commits legados.

hotsoft-desenv2 · 2016-11-24T12:54:06Z

app/models/report_template.rb

-    value.nil? ? {} : Marshal.load(value)
+    begin
+      value.nil? ? {} : Marshal.load(value)
+    rescue TypeError


Tratamento em relação a nulidade do valor

Aqui você está abafando uma exceção. Isto não é uma boa prática pois apesar de resolver algum caso específico que você precisou resolver poderá abafar exceções que deveriam aparecer para o programador. Isto pode atrapalhar em debugs.
Poderia alterar e somente abafar nos casos em que necessita?

O commit original somente descreve: tratamento de valor nulo.
Se desejar posso remover esta alteração e testar um por um dos relatórios.

No na época(2013) não foi documentado não sei se a alteração
pode tratar situação específicas ou influenciadas pela combinação de filtros.

Se eu remover apesar dos testes locais ainda haverá a possibilidade do erro ocorrer no cliente. Neste caso seria interessante manter esta alteração?

Como boa prática peço que remova a alteração. Se for necessário de fato o ideal é criar em seu projeto uma subclasse desta classe que faça isto e a use somente no seu projeto. Como se trata de uma biblioteca usada em vários projetos não temos como aceitar esta mudança na classe comum.

Hum. Entendi.
Neste caso irei refatorar e em breve subo as alterações.

No entanto devido a rotina interna irei alinhar um prazo.
Não conseguirei fazer neste momento.

Portanto o pull request vai demorar um tempo para ser realizado novamente.

hotsoft-desenv2 · 2016-11-24T12:54:39Z

lib/generators/prawn_report/install/templates/20131107172133_add_excluir_to_report_template.rb

+  def self.up
+    add_column :report_templates, :excluir, :boolean
+  end
+


Migration relativa a coluna que agora controla a exclusão.

hotsoft-desenv2 · 2016-11-24T12:55:21Z

lib/prawn_report_seeds.rb

-    f = AcFilterDef.find_by_name(params.delete("filter_name"))
-    if f
-      params["ac_filter_def_id"] = f.id
+  r = ReportTemplate.find_by_name(params["name"])


Ajustes para o novo comportamento relativo a "exclusão".

Favor detalhar esta lógica de exclusão. O que ela resolve? Por que foi implementada?

Alterado para atualizar ao invés de excluir. Para não perder relação entre os relatórios e os laboratórios. Só exibir relatórios para laboratórios específicos.

Como se trata de uma implementação específica para o seu projeto peço que realize em classes fora do contexto da biblioteca, no seu projeto por exemplo. Ou então que insira este comportamento porém de forma configurável e mantendo o padrão como era anteriormente.

hotsoft-desenv2 · 2016-11-24T12:59:52Z

Bom dia @acras,

realizado além da limpeza alguns comentários adicionais.
Portei manualmente conforme nosso fork local.
(Ajustes realizados no passado)

Em novas alterações farei com Pull requests mais frequentes com
comentários mais concisos e sem alterações não relacionadas ao comentário. (Mantive os comentários originais do nosso Fork).

No entanto creio que agora este pull request esteja passível de ser aplicado.
Aguardo sua nova revisão.

acras

Obrigado pela evolução Wellington, favor revisar os pontos que levantei e postar novamete.

acras · 2016-11-25T19:55:24Z

app/models/report_template.rb

-    value.nil? ? {} : Marshal.load(value)
+    begin
+      value.nil? ? {} : Marshal.load(value)
+    rescue TypeError


Aqui você está abafando uma exceção. Isto não é uma boa prática pois apesar de resolver algum caso específico que você precisou resolver poderá abafar exceções que deveriam aparecer para o programador. Isto pode atrapalhar em debugs.
Poderia alterar e somente abafar nos casos em que necessita?

acras · 2016-11-25T20:05:01Z

lib/ac_filters_utils.rb

      sql_to_execute = ActiveRecord::Base.send(:sanitize_sql_array, conditions)
      r = []
-      ActiveRecord::Base.connection.instance_exec(sql_to_execute).each(:as => :hash) {|i| r << i}
+      ActiveRecord::Base.connection.instance_exec(sql_to_execute).each(as :hash) {|i| r << i}


O que faz esta alteração?

acras · 2016-11-25T20:06:17Z

lib/prawn_report_seeds.rb

-    f = AcFilterDef.find_by_name(params.delete("filter_name"))
-    if f
-      params["ac_filter_def_id"] = f.id
+  r = ReportTemplate.find_by_name(params["name"])


Favor detalhar esta lógica de exclusão. O que ela resolve? Por que foi implementada?

acras · 2016-11-25T20:09:08Z

repo/reports/listing.rb

  class Listing < Report

-    #alias :super_new_page :new_page
+    alias :super_new_page :new_page


Não entendi o objetivo desta alteração.

acras · 2016-11-25T20:12:25Z

Boa tarde Wellington, acabo de enviar para você solicitações de alteração e mais informações. Aguardo novo pull request.

…

-- Ricardo Acras ricardo@acras.com.br www.focuslojas.com.br www.focusnfe.com.br 2016-11-24 10:59 GMT-02:00 Hotsoft_desenv2 <notifications@github.com>:

Bom dia @acras <https://github.com/acras>, realizado além da limpeza alguns comentários adicionais. Portei manualmente conforme nosso fork local. (Ajustes realizados no passado) Em novas alterações farei com Pull requests mais frequentes com comentários mais concisos e sem alterações não relacionadas ao comentário. (Mantive os comentários originais do nosso Fork). No entanto creio que agora este pull request esteja passível de ser aplicado. Aguardo sua nova revisão. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAt0EYha9TsUl3aMroG6SdvpuWJjpbw5ks5rBYpJgaJpZM4K69yQ> .

hotsoft-desenv2

Realizei novos comentários. Favor validar.

hotsoft-desenv2 · 2016-11-28T12:19:49Z

repo/reports/listing.rb

  class Listing < Report

-    #alias :super_new_page :new_page
+    alias :super_new_page :new_page


Segue o Ticket: http://192.168.200.44:3000/issues/22060

hotsoft-desenv2 · 2016-11-28T12:27:05Z

lib/prawn_report_seeds.rb

-    f = AcFilterDef.find_by_name(params.delete("filter_name"))
-    if f
-      params["ac_filter_def_id"] = f.id
+  r = ReportTemplate.find_by_name(params["name"])


Alterado para atualizar ao invés de excluir. Para não perder relação entre os relatórios e os laboratórios. Só exibir relatórios para laboratórios específicos.

hotsoft-desenv2 · 2016-11-28T12:41:29Z

lib/ac_filters_utils.rb

      sql_to_execute = ActiveRecord::Base.send(:sanitize_sql_array, conditions)
      r = []
-      ActiveRecord::Base.connection.instance_exec(sql_to_execute).each(:as => :hash) {|i| r << i}
+      ActiveRecord::Base.connection.instance_exec(sql_to_execute).each(as :hash) {|i| r << i}


Só troca de sintaxe mesmo. Posso remover esta alteração.
Trouxe dos commits legados.

hotsoft-desenv2 · 2016-11-28T12:43:18Z

repo/reports/listing.rb

  class Listing < Report

-    #alias :super_new_page :new_page
+    alias :super_new_page :new_page


É utilizado em alguns de nossos relatórios.
Alteração realizada para evitar problema com os relatórios que utilizam este alias.
Conforme o Ticket citado acima.

hotsoft-desenv2 · 2016-11-28T12:52:30Z

app/models/report_template.rb

-    value.nil? ? {} : Marshal.load(value)
+    begin
+      value.nil? ? {} : Marshal.load(value)
+    rescue TypeError


O commit original somente descreve: tratamento de valor nulo.
Se desejar posso remover esta alteração e testar um por um dos relatórios.

No na época(2013) não foi documentado não sei se a alteração
pode tratar situação específicas ou influenciadas pela combinação de filtros.

Se eu remover apesar dos testes locais ainda haverá a possibilidade do erro ocorrer no cliente. Neste caso seria interessante manter esta alteração?

hotsoft-desenv2 · 2016-11-28T13:44:10Z

Bom dia @acras,

acabo de enviar novos comentários complementando as informações anteriores.
Segue então para uma nova análise.

Obrigado.

acras · 2016-11-28T14:02:11Z

Enviados mais comentários...

wellington1993 added 10 commits November 23, 2016 15:27

Ticket_id #52283 - seed não exlcui mais relatórios ja existentes.

64afab2

Ticket_id #52283 - Destroy, para chamar evento...

2bd352f

Ticket_id #52283 - Tratamento de nulidade

ba6536e

Ticket_id #52283 - Bubstituido o marshal pelo yaml

97ef69d

Ticket_id #52283 - new page é chamado de cabeçalhos, rodapés e agrupa…

da6cbe3

…mentos.

Ticket_id #52283 - YAML só é o serializador padrão a partir do rails 3.

7744702

Ticket_ID: #22060 - Erro nos relatórios chamando super_new_page

c822c9c

Ticket_ID: #51605 - Erro conversao de tipo

548ed45

Ticket_id #52283 - Ajuste no gemspec, correção sintaxe e especificação

e64f583

Ticket_id #52283 - Ajuste incompatibilidade com versões antigas do Fr…

f4c23a1

…amework

hotsoft-desenv2 closed this Nov 23, 2016

hotsoft-desenv2 reopened this Nov 23, 2016

Ticket_id #52283 - Limpeza. Removido código comentado e código legado…

a5ff925

…. Removido Ticket removido do fonte.

hotsoft-desenv2 commented Nov 24, 2016

View reviewed changes

acras requested changes Nov 25, 2016

View reviewed changes

hotsoft-desenv2 commented Nov 28, 2016

View reviewed changes

Ticket_id #52283 - Alinhamento com o Fork da Hotsoft #3

Are you sure you want to change the base?

Ticket_id #52283 - Alinhamento com o Fork da Hotsoft #3

Uh oh!

Conversation

hotsoft-desenv2 commented Nov 23, 2016

Uh oh!

acras commented Nov 24, 2016

Uh oh!

hotsoft-desenv2 commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

hotsoft-desenv2 Nov 24, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

hotsoft-desenv2 commented Nov 24, 2016

Uh oh!

acras left a comment

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!

acras commented Nov 25, 2016 via email

Uh oh!

hotsoft-desenv2 left a comment

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!

hotsoft-desenv2 commented Nov 24, 2016 •

edited

Loading

hotsoft-desenv2 Nov 24, 2016 •

edited

Loading