Skip to content

Conversation

@Panioglo
Copy link
Member

@Panioglo Panioglo commented Aug 18, 2016

Fixes #243 #241


This change is Reviewable

@Chocksy
Copy link
Member

Chocksy commented Aug 19, 2016

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


app/models/issue.rb, line 25 [r1] (raw file):

  def increment_aggregate
    ErrorStore::Aggregates.new.handle_aggregates(self)

this should be ErrorStore::Aggregates.new(self).handle_aggregates


app/models/issue.rb, line 81 [r1] (raw file):

        key = 'rails'
      end
      "#{key.capitalize}/#{modules[key.to_sym]}" if defined? key

why is this here? Why do we need rails instead of ruby as key? What are you trying to do?


app/views/errors/show.html.haml, line 47 [r1] (raw file):

                - unless @error.aggregates.blank?
                  - @error.aggregates.each do |record|
                    = render partial: 'errors/aggregations', locals: { data: record}

you can use collection here render partial: 'errors/aggregations', collection: @error.aggregates


db/schema.rb, line 78 [r1] (raw file):

  create_table "aggregates", force: :cascade do |t|
    t.integer  "grouped_issue_id", :index=>{:name=>"index_aggregates_on_grouped_issue_id"}, :foreign_key=>{:references=>"grouped_issues", :name=>"fk_aggregates_grouped_issue_id", :on_update=>:restrict, :on_delete=>:cascade}
    t.string   "agg_type"

you should change this to name so it does not conflict with the sql reserved values.


lib/error_store.rb, line 182 [r1] (raw file):

      end
    end
  end

woooo no man don't do this. Create a new file and put the code there don't add the class in the error_store file...


Comments from Reviewable

@Panioglo
Copy link
Member Author

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


app/models/issue.rb, line 81 [r1] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

why is this here? Why do we need rails instead of ruby as key? What are you trying to do?

trying to make them similar to what i've seen in the video

Comments from Reviewable

@Panioglo
Copy link
Member Author

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


app/models/issue.rb, line 81 [r1] (raw file):

Previously, Panioglo wrote…

trying to make them similar to what i've seen in the video

http://pasteboard.co/awBJfqH04.png

Comments from Reviewable

@Chocksy
Copy link
Member

Chocksy commented Aug 19, 2016

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


app/models/issue.rb, line 25 [r1] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

this should be ErrorStore::Aggregates.new(self).handle_aggregates

I would change this from `increment_aggregate` to `refresh_aggregates` or something similar. We should also handle/call this using sidekiq

app/models/issue.rb, line 81 [r1] (raw file):

Previously, Panioglo wrote…

http://pasteboard.co/awBJfqH04.png

We don't need to make it similar data it's ok if it's just ruby.

Comments from Reviewable

@Panioglo
Copy link
Member Author

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


app/models/issue.rb, line 81 [r1] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

We don't need to make it similar data it's ok if it's just ruby.

the ​ruby key is not present in the modules hash as I​ shown you in the above picture,​ in ​other way I should remove versions aggregate

Comments from Reviewable

@Chocksy
Copy link
Member

Chocksy commented Aug 22, 2016

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


app/models/issue.rb, line 27 [r3] (raw file):

    cache_key = self.object_id.to_s
    Rails.cache.write(cache_key, self)
    AggregatesWorker.perform_async(cache_key)

You don't need to do this. You can send the issue.id to perfom and thats it. There is no need for caching as you are not storing anything really. You can do a find in the worker for the issue when the job runs.


app/models/issue.rb, line 156 [r3] (raw file):

    def perform(cache_key)
      record = Rails.cache.read(cache_key)
      ErrorStore::Aggregates.new(record).handle_aggregates

Let's add workers in their folder in app. https://github.com/mperham/sidekiq/wiki/Getting-Started


Comments from Reviewable

@Panioglo
Copy link
Member Author

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


app/models/issue.rb, line 27 [r3] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

You don't need to do this. You can send the issue.id to perfom and thats it. There is no need for caching as you are not storing anything really. You can do a find in the worker for the issue when the job runs.

this will also help me to avoid some issues with caching​, thanks

Comments from Reviewable

@codecov-io
Copy link

codecov-io commented Aug 22, 2016

Current coverage is 96.87% (diff: 89.43%)

Merging #260 into master will decrease coverage by 0.11%

@@             master       #260   diff @@
==========================================
  Files           170        174     +4   
  Lines          5687       5862   +175   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5516       5679   +163   
- Misses          171        183    +12   
  Partials          0          0          

Powered by Codecov. Last update 116f528...15b3494

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants