Skip to content
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

added a filters tab in the errors sidebar #228

Merged
merged 7 commits into from
Aug 8, 2016
Merged

added a filters tab in the errors sidebar #228

merged 7 commits into from
Aug 8, 2016

Conversation

Panioglo
Copy link
Member

@Panioglo Panioglo commented Aug 2, 2016

Fixes #209


This change is Reviewable

@Chocksy
Copy link
Member

Chocksy commented Aug 2, 2016

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


app/assets/javascripts/pages/errors.js, line 26 [r1] (raw file):

    window.location.href = uri;
  });

We definitely don't want to do this. 100% against parsing urls and adding them like this. It seems more ok to me to just submit the form as you said it was happening.


app/controllers/errors_controller.rb, line 54 [r1] (raw file):

      if params[:intercom]
        begin
          current_website.intercom_integration.driver.send_message(params[:users], params[:message])

why the hell did you do this?


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

  /     %label.radio-inline
  /       %input{:name => "status", :type => "radio"}>/
  /       Resolved

Don't leave commented code in please.


Comments from Reviewable

@Chocksy
Copy link
Member

Chocksy commented Aug 2, 2016

@Panioglo fix the specs also please!

@Panioglo
Copy link
Member Author

Panioglo commented Aug 3, 2016

Review status: 6 of 13 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


app/controllers/errors_controller.rb, line 54 [r1] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

why the hell did you do this?

this is a mistake, I will explain it to you later

Comments from Reviewable

@Chocksy
Copy link
Member

Chocksy commented Aug 3, 2016

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


Gemfile, line 40 [r2] (raw file):

gem 'jquery-turbolinks'
gem 'momentjs-rails'
gem 'bootstrap3-datetimepicker-rails', '~> 4.14.30'

Let's use this instead of this gem: http://www.daterangepicker.com/


Gemfile.lock, line 485 [r2] (raw file):

BUNDLED WITH
   1.12.5

be careful and remove these lines from gemfile.lock when they are added.


app/assets/javascripts/pages/errors.js, line 21 [r2] (raw file):

  }
  return uri
}

please remove these i don't want regex for the simple search feature we want to add


Comments from Reviewable

@codecov-io
Copy link

codecov-io commented Aug 8, 2016

Current coverage is 96.99% (diff: 98.73%)

Merging #228 into master will decrease coverage by 0.04%

@@             master       #228   diff @@
==========================================
  Files           172        170     -2   
  Lines          5559       5616    +57   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5394       5447    +53   
- Misses          165        169     +4   
  Partials          0          0          

Powered by Codecov. Last update 4ed774a...14d9da0

@Panioglo
Copy link
Member Author

Panioglo commented Aug 8, 2016

Review status: 3 of 23 files reviewed at latest revision, 6 unresolved discussions.


app/assets/javascripts/pages/errors.js, line 26 [r1] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

We definitely don't want to do this. 100% against parsing urls and adding them like this. It seems more ok to me to just submit the form as you said it was happening.

Done.

app/assets/javascripts/pages/errors.js, line 21 [r2] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

please remove these i don't want regex for the simple search feature we want to add

Done.

app/controllers/errors_controller.rb, line 54 [r1] (raw file):

Previously, Panioglo wrote…

this is a mistake, I will explain it to you later

Done.

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

Previously, Chocksy (Ciocanel Razvan) wrote…

Don't leave commented code in please.

Done.

Comments from Reviewable

@Panioglo
Copy link
Member Author

Panioglo commented Aug 8, 2016

Review status: 3 of 23 files reviewed at latest revision, 6 unresolved discussions.


Gemfile.lock, line 485 [r2] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

be careful and remove these lines from gemfile.lock when they are added.

Done.

Gemfile, line 40 [r2] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

Let's use this instead of this gem: http://www.daterangepicker.com/

Done.

Comments from Reviewable

@Chocksy
Copy link
Member

Chocksy commented Aug 8, 2016

Reviewed 19 of 21 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


app/controllers/errors_controller.rb, line 26 [r4] (raw file):

    errors = current_website.grouped_issues.order('last_seen DESC')
    resolve if params[:commit] == 'Resolve'
    unresolve if params[:commit] == 'Unresolve'

Why are you doing this here?


app/controllers/errors_controller.rb, line 32 [r4] (raw file):

      page = (position.to_f/errors_per_page).ceil
      errors = errors.with_status(params[:status].to_sym) unless params[:status].blank?
    elsif params[:commit] == 'search-button' && !params[:search].blank?

don't use ! just use present?


app/controllers/errors_controller.rb, line 34 [r4] (raw file):

    elsif params[:commit] == 'search-button' && !params[:search].blank?
      errors = matching_elements
    elsif !params[:datepicker].blank?

elsif params[:datepicker].present?


app/views/errors/_error.html.haml, line 2 [r4] (raw file):

#grouped-issues.groups
  = link_to error_url(id: error, tab: 'default') do

Why did you add id here?


Comments from Reviewable

@Panioglo
Copy link
Member Author

Panioglo commented Aug 8, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


app/controllers/errors_controller.rb, line 26 [r4] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

Why are you doing this here?

we get commit params == Resolve/Unresolve when we try to resolve or unresolve a group​. I used all these checks to keep the code ​clear

app/controllers/errors_controller.rb, line 32 [r4] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

don't use ! just use present?

.present? is more buggy than blank? cuz params[:search] can be equal to " ".. if param is a blank string.. present method still returns true

app/views/errors/_error.html.haml, line 2 [r4] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

Why did you add id here?

I will remove the id, the result will be the same

Comments from Reviewable

@Panioglo
Copy link
Member Author

Panioglo commented Aug 8, 2016

Review status: 22 of 23 files reviewed at latest revision, 4 unresolved discussions.


app/controllers/errors_controller.rb, line 34 [r4] (raw file):

Previously, Chocksy (Ciocanel Razvan) wrote…

elsif params[:datepicker].present?

blank? replaces (params[:datepicker].present? && params[:datepicker] != "" )

Comments from Reviewable

@Chocksy
Copy link
Member

Chocksy commented Aug 8, 2016

:lgtm:


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


app/controllers/errors_controller.rb, line 32 [r4] (raw file):

Previously, Panioglo wrote…

.present? is more buggy than blank? cuz params[:search] can be equal to " ".. if param is a blank string.. present method still returns true

present? is not buggy that's the way it should work :))

app/controllers/errors_controller.rb, line 34 [r4] (raw file):

Previously, Panioglo wrote…

blank? replaces (params[:datepicker].present? && params[:datepicker] != "" )

I know what it does but i don't see how params[:datepicker] would be empty string. If you say it can then we can go with this code. I'll approve it.

Comments from Reviewable

@Chocksy Chocksy merged commit 0b809fd into master Aug 8, 2016
@Chocksy Chocksy deleted the feat-filters branch August 8, 2016 13:01
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.

3 participants