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.

Sign up for GitHub

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

Jump to bottom

Place /stats routes behind login check, for performance reasons #9536

Merged
merged 5 commits into from
May 10, 2021
Merged

Place /stats routes behind login check, for performance reasons #9536

merged 5 commits into from
May 10, 2021

Conversation

jywarren
Copy link
Member

await approval in #9002, but this is a pretty simple change!

@jywarren
Place /stats routes behind login check, for performance reasons
cf0c0e1 
await approval in #9002, but this is a pretty simple change!
@gitpod-io
Copy link

gitpod-io bot commented Apr 20, 2021

@jywarren jywarren mentioned this pull request Apr 20, 2021
Closed
@codecov
Copy link

codecov bot commented Apr 20, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@bd0bc11). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9536   +/-   ##
=======================================
  Coverage        ?   81.35%           
=======================================
  Files           ?       98           
  Lines           ?     5928           
  Branches        ?        0           
=======================================
  Hits            ?     4823           
  Misses          ?     1105           
  Partials        ?        0

@jywarren
Copy link
Member Author

This probably broke some tests which expect to be able to get to /stats -- should be a relatively easy fix!

@jywarren
Copy link
Member Author

jywarren commented May 4, 2021

Yes - changes needed on:

  •     test/functional/stats_controller_test.rb:30:in `block in <class:StatsControllerTest>'

  •     test/integration/public_pages_test.rb:80:in `block in <class:PublicPagesTest>'

@gitpod-io
Copy link

gitpod-io bot commented May 4, 2021

@jywarren
Copy link
Member Author

jywarren commented May 4, 2021 

Progress: |======== FAIL I18nTest#test_should_choose_i18n_for_notes/stats (200.61s)
        Expected at least 1 element matching "h2", found 0..
        Expected 0 to be >= 1.
        test/integration/I18n_test.rb:509:in `block (2 levels) in <class:I18nTest>'
        test/integration/I18n_test.rb:500:in `each'
        test/integration/I18n_test.rb:500:in `block in <class:I18nTest>'

Progress: |====== FAIL StatsControllerTest#test_should_assign_correct_value_to_graph_comments_on_GET_stats (21.89s)
        --- expected
        +++ actual
        @@ -1 +1 @@
        -nil
        +{1617148800000.0=>0, 1617753600000.0=>0, 1618358400000.0=>0, 1618963200000.0=>0, 1619568000000.0=>0}
        test/functional/stats_controller_test.rb:33:in `block in <class:StatsControllerTest>'

 FAIL StatsControllerTest#test_should_load_stats_range_query (22.04s)
        Expected response to be a <2XX: success>, but was a <302: Found> redirect to <http://test.host/login?return_to=/stats>
        Response body: <html><body>You are being <a href="http://test.host/login?return_to=/stats">redirected</a>.</body></html>
        test/functional/stats_controller_test.rb:40:in `block in <class:StatsControllerTest>'

jywarren
jywarren commented May 4, 2021
View reviewed changes
test/functional/stats_controller_test.rb
@@ -32,6 +35,7 @@ def setup
end

test 'should load stats range query' do
UserSession.create(users(:bob)) # we now require login for this page for load reasons
get :index
assert_response :success
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why should this be a 302 redirect???

@cesswairimu
Copy link
Collaborator

cesswairimu commented May 5, 2021 •
edited
Loading

Hi Jeff,
I think I found the prob. The integration test that was failing after inspecting I found that it was returning an flash message You must be logged in to access this page You must be logged in to access this page adding a session on the test solved it.

The functional one error was Authlogic::Session::Activation::NotActivatedError: You must activate the Authlogic::Session::Base.controller with a controller object before creating objects fixed by adding activate_authlogic under setup to activate the sessions created in the tests

hopefully we are all green now 🤞 thanks

@codeclimate
Copy link

codeclimate bot commented May 5, 2021

Code Climate has analyzed commit bc97636 and detected 0 issues on this pull request.

View more on Code Climate.

@jywarren jywarren merged commit 11b01ce into main May 10, 2021
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
@jywarren @cesswairimu @reginaalyssa
Place /stats routes behind login check, for performance reasons (publ…
29989fe 
…iclab#9536)

* Place /stats routes behind login check, for performance reasons

await approval in publiclab#9002, but this is a pretty simple change!

* Update public_pages_test.rb

* Update stats_controller_test.rb

* Update stats_controller_test.rb

* fix failing tests

Co-authored-by: Cess <[email protected]>
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
@jywarren @cesswairimu @billymoroney1
Place /stats routes behind login check, for performance reasons (publ…
915c53e 
…iclab#9536)

* Place /stats routes behind login check, for performance reasons

await approval in publiclab#9002, but this is a pretty simple change!

* Update public_pages_test.rb

* Update stats_controller_test.rb

* Update stats_controller_test.rb

* fix failing tests

Co-authored-by: Cess <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jywarren @cesswairimu