Skip to content
This repository was archived by the owner on Nov 14, 2018. It is now read-only.

Conversation

@openjck
Copy link
Contributor

@openjck openjck commented Mar 2, 2016

No description provided.

@groovecoder
Copy link
Collaborator

Some flake8 fixes to make as reported by Travis:

./dashboard/settings.py:18:1: E302 expected 2 blank lines, found 1
./push/urls.py:1:1: F401 'include' imported but unused
./push/urls.py:1:1: F401 'patterns' imported but unused
./push/views.py:6:1: E302 expected 2 blank lines, found 1
./push/views.py:12:80: E501 line too long (80 > 79 characters)
./push/views.py:22:1: E302 expected 2 blank lines, found 1
./push/views.py:28:80: E501 line too long (81 > 79 characters)

'css/main.css',
'css/layout.css',
'css/forms.css',

Copy link
Collaborator

Choose a reason for hiding this comment

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

extra lines here 😐

@openjck openjck force-pushed the issue-37-vapid-validation branch from bdfeaa7 to b57de7f Compare March 2, 2016 22:01

{% block content %}
{% if not request.user.is_authenticated %}
{% include 'dashboard/includes/authenticate.html' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than include the authenticate template at this URL, we should wrap the views in login_required to bounce the user to the login page and bring them back here. Can be filed as a follow-up issue/TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wanted to use that approach instead. I was having trouble implementing it, but I'll give it another look.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't burn yourself with too much time on it; happy to file an issue for next PR/merge/deploy.

@groovecoder
Copy link
Collaborator

Looking good. We should also make the <td>{{ app.name }}</td> a link to the push.details view for the app, so it's easy to get there.

@groovecoder
Copy link
Collaborator

Cool, with flake8 fixes and re-centering of the new iframe, I'm happy to merge this if we file the rest as follow-up issues.

@groovecoder groovecoder assigned openjck and unassigned groovecoder Mar 2, 2016
@openjck openjck force-pushed the issue-37-vapid-validation branch from b57de7f to 77ae4b1 Compare March 2, 2016 22:17
@openjck
Copy link
Contributor Author

openjck commented Mar 2, 2016

Looking good. We should also make the {{ app.name }} a link to the push.details view for the app, so it's easy to get there.

Done. I'll file a follow-up for the @login_required decorator.

groovecoder added a commit that referenced this pull request Mar 3, 2016
@groovecoder groovecoder merged commit 62e14a2 into master Mar 3, 2016
@groovecoder groovecoder deleted the issue-37-vapid-validation branch March 3, 2016 16:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants