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

implement naive self-service banners for python.org #1413

Merged
merged 4 commits into from Apr 18, 2019
Merged

implement naive self-service banners for python.org #1413

merged 4 commits into from Apr 18, 2019

Conversation

@ewdurbin
Copy link
Member

@ewdurbin ewdurbin commented Apr 11, 2019

Allows management of banners displayed across python.org via Django admin.

Screen Shot 2019-04-11 at 10 24 09 AM

Screen Shot 2019-04-11 at 10 24 12 AM

Screen Shot 2019-04-11 at 10 24 25 AM

ewdurbin added 2 commits Apr 11, 2019

@register.simple_tag
def render_active_banner(psf_pages_only=True):
if not psf_pages_only:

This comment has been minimized.

@berkerpeksag

berkerpeksag Apr 12, 2019
Member

If I understood this correctly, the following line would cover both if and else statements:

banner = Banner.objects.filter(active=True, psf_pages_only=psf_pages_only).first()

This comment has been minimized.

@ewdurbin

ewdurbin Apr 18, 2019
Author Member

little trickier than that, since we accept either value for the PSF pages. so filtering on psf_pages_only makes general banners not show.

class BannerAdmin(admin.ModelAdmin):
list_display = ("title", "active", "psf_pages_only")

admin.site.register(Banner, BannerAdmin)

This comment has been minimized.

@berkerpeksag

berkerpeksag Apr 12, 2019
Member

Style nit: We are trying to use the @admin.site.register() decorator version in new code.

from django.db import migrations, models


class Migration(migrations.Migration):

This comment has been minimized.

@berkerpeksag

berkerpeksag Apr 12, 2019
Member

Do you plan to squash these two migrations when design of the Banner model is finished?

This comment has been minimized.

@ewdurbin

ewdurbin Apr 18, 2019
Author Member

aye

@@ -38,6 +39,8 @@ <h1 class="call-to-action">Download the latest version of Python</h1>
{% block content %}
<div class="row download-list-widget">

{% render_active_banner False %}

This comment has been minimized.

@berkerpeksag

berkerpeksag Apr 12, 2019
Member

Nit: Passing psf_pages_only=False would increase readability here.

This comment has been minimized.

@ewdurbin

ewdurbin Apr 18, 2019
Author Member

again, will be addressed with excellent suggestion from jaap3

@@ -26,6 +27,8 @@
{% block breadcrumbs %}
{% load sitetree %}

{% render_active_banner True %}

This comment has been minimized.

@berkerpeksag

berkerpeksag Apr 12, 2019
Member

Nit: Did Django complained without passing an argument? I don't have a computer to test at the moment, but we might just use {% render_active_banner %} here.

This comment has been minimized.

@ewdurbin

ewdurbin Apr 18, 2019
Author Member

with suggestion from jaap3, this isn't necessary. I was just being explicit :)



@register.simple_tag
def render_active_banner(psf_pages_only=True):

This comment has been minimized.

@jaap3

jaap3 Apr 13, 2019
Contributor

What about having two template tags render_active_banner and render_active_psf_banner instead of adding a boolean argument? This would also make it clearer what is going on in the template.

This comment has been minimized.

@ewdurbin

ewdurbin Apr 18, 2019
Author Member

great suggestion, thank you!

if not psf_pages_only:
banner = Banner.objects.filter(active=True, psf_pages_only=psf_pages_only).first()
else:
banner = Banner.objects.filter(active=True).first()

This comment has been minimized.

@jaap3

jaap3 Apr 13, 2019
Contributor

It would be nice to have a custom QuerySet/Manager that provides an active() method (i.e. Banner.objects.active().first())

'title': banner.title,
'link': banner.link,
}
return tmpl.render(ctx)

This comment has been minimized.

ewdurbin added 2 commits Apr 18, 2019
@ewdurbin ewdurbin merged commit cdb0b8b into master Apr 18, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ewdurbin ewdurbin deleted the banners branch Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.