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

Search pagination #41

Merged
merged 4 commits into from
Apr 21, 2017
Merged

Search pagination #41

merged 4 commits into from
Apr 21, 2017

Conversation

dianaboiangiu
Copy link
Contributor

No description provided.

@@ -60,7 +60,7 @@


# Search
url(r'^search/$', views.SearchView.as_view(), name='search'),
url(r'^search/page/(?P<page>\d+)/$', views.SearchView.as_view(), name='search'),
Copy link
Contributor

Choose a reason for hiding this comment

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

page should be a GET parameter, not a required url parameter.

@@ -24,6 +24,7 @@
from gemet.thesaurus.utils import search_queryset, exp_decrypt, is_rdf
from gemet.thesaurus import DEFAULT_LANGCODE, NR_CONCEPTS_ON_PAGE
from gemet.thesaurus import NS_ID_VIEW_MAPPING
from django.core.paginator import Paginator
Copy link
Contributor

Choose a reason for hiding this comment

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

Place the import next to other django imports

else:
return self.form_invalid(form)
else:
return super(SearchView, self).get(request, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

All this processing doesn't look right. Maybe inherit from ListView and FormMixin?

<a href="{% url 'search' language.code concepts.next_page_number %}?query={{ query }}">next</a>
{% endif %}
</span>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

One should be able to go to any results page directly. Please use the same layout as the other paginated views use (e.g. Alphabetic Listing).

@dianaboiangiu dianaboiangiu force-pushed the search_pagination branch 2 times, most recently from 8357a1b to bd6ee7e Compare April 11, 2017 13:03
from django.core.urlresolvers import reverse
from django.db.models import Q
from django.views import View
from django.views.generic import TemplateView, ListView
from django.views.generic.detail import DetailView
from django.views.generic.edit import FormView
from django.views.generic.edit import FormView, FormMixin
Copy link
Contributor

Choose a reason for hiding this comment

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

FormMixin imported but unused.

self.query = form.cleaned_data['query']
self.concepts = search_queryset(
self.query,
self.language,
status_values=self.status_values,
)
page = self.request.GET.get('page', 1)
paginator = Paginator(self.concepts, 25)
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace hardcoded value 25 with NR_CONCEPTS_ON_PAGE

context = self.get_context_data(form=form, paginator=paginator)
page_number = self.concepts.number
total_pages = len(self.concepts.paginator.page_range)
distance_number = 25
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 25 for distance? The other views use 9. Define a constant and use it in both places

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 75.726% when pulling 7e76348 on search_pagination into 530ca91 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 75.671% when pulling 2ea5501 on search_pagination into 530ca91 on master.

@iuliachiriac iuliachiriac merged commit 785ec13 into master Apr 21, 2017
@iuliachiriac iuliachiriac deleted the search_pagination branch April 24, 2017 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants