-
Notifications
You must be signed in to change notification settings - Fork 62
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
Fix profiles controller index #1350
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 Only 2 questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work :) Only some suggestions for better descriptions for describe blocks (as defined in https://www.betterspecs.org/).
Also: everything seems to work, I don' yet understand how one spec works (see comment).
config/initializers/pagy.rb
Outdated
@@ -180,7 +180,7 @@ | |||
# Overflow extra: Allow for easy handling of overflowing pages | |||
# See https://ddnexus.github.io/pagy/docs/extras/overflow | |||
require 'pagy/extras/overflow' | |||
Pagy::DEFAULT[:overflow] = :last_page # (other options: :empty_page and :exception) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, diese Änderung habe ich ja schon gemacht. Wäre besser, du rebased auf Master. Ich hatte dort `:last_page noch als Option in den Kommentar geschrieben.
expect(assigns(:aggs_states)).to eq nil | ||
end | ||
end | ||
context 'single profile tests' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ich finde den Kontext hier unpassend. Es handelt sich ja nicht nur um Tests mit "single profile". Kann einfach weg.
it 'is not permitted for unauthorized not signed in profile' do | ||
get :show, params: { id: profile_unpublished.id } | ||
expect(response).to redirect_to("/#{I18n.locale}/profiles") | ||
describe 'test index action' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe '#index' do
it 'should be seen by all profiles' do | ||
get :show, params: { id: profile_published.id } | ||
expect(response).to render_template(:show) | ||
describe 'category id search' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe '#search_with_category_id' do
|
||
it 'renders edit view' do | ||
expect(response).to render_template(:edit) | ||
describe 'tag filter search' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe '#search_with_tags' do
|
||
describe 'update profile action' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe '#update' do
sign_in ada | ||
put :update, params: { id: ada.id, profile: { email: ' ' } } | ||
end | ||
context 'when invalid params are supplied' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context 'with invalid params' do
|
||
describe 'update profile action' do | ||
context 'when updating own profile with valid params' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context 'with valid params' do
|
||
describe 'destroy profile action' do | ||
describe 'destroy profile action' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe '#destroy' do
get :index, params: { page: 2 } | ||
expect(assigns(:records).count).to eq 2 | ||
profiles_page_2 = assigns(:records) | ||
expect(profiles_page_2 & profiles_page_1).to eq [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sieht elegant aus, aber eigentlich verstehe ich nicht genau, was & hier macht. Ist das sowas wie eine Schnittmenge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jep it checks for an intersection. I wanted to check that we shall no profile twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
In the index profile view we show all profiles randomize and paginated which leads to showing profiles twice or even more on different pages. This fixes this.
https://speakerinnen.org/en/profiles