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

[issue1003] Improve style for create and main pages, add create form #309

Merged
merged 10 commits into from Aug 7, 2014
@@ -0,0 +1,137 @@
# This file is part of OpenHatch.
# Copyright (C) 2014 Elana Hashman
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

import mysite.bugsets.models

import django.forms
from django.core.validators import URLValidator
from django.core.exceptions import ObjectDoesNotExist


class BugsForm(django.forms.Form):
event_name = django.forms.CharField(max_length=200)
buglist = django.forms.CharField(
widget=django.forms.Textarea,
label='Enter some bugs URLs here, each separated by a newline:',
)

@staticmethod
def bug_list_from_set(bugset):

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

It's slightly sad to have this method, given that it does a SQL query rather than returning a nice joyous Django lazy QuerySet.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 11, 2014

Author Member

Eh, maybe. I'd be +0 to changing this. Needs to be enumerated through at some point.

return [bug.url for bug in bugset.bugs.all()]

# What is happening here?!

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

This is often better as a docstring. +1 to having all this commentary, though!

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 11, 2014

Author Member

Easy fix to make!

I'm not crazy about having these helpers as static methods but it seemed to be the choice that made the most sense at the time.

# 1. Split the hunk of text on newlines
# 2. Strip all the strings of leading/trailing whitespace
# 3. Filter out all empty strings by passing the first-order function
# "bool" (to see why: bool(x) == False if and only if x == "")
@staticmethod
def url_list_from_bugtext(bugtext):
return filter(bool, [x.strip() for x in bugtext.split("\n")])

def __init__(self, *args, **kwargs):
self.pk = kwargs.get('pk')

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

Might as well leave a comment about how we know this is similar to ModelForm but we haven't ported this to that.

self.object = None

if self.pk is not None:
kwargs.pop('pk')
self.object = mysite.bugsets.models.BugSet.objects.get(pk=self.pk)

if kwargs.get('data') is None:
data = {
'event_name': self.object.name,
'buglist': "\n".join(
BugsForm.bug_list_from_set(self.object)),
}
kwargs['data'] = data

super(BugsForm, self).__init__(*args, **kwargs)

def clean_buglist(self):
# If this corresponds to an existing set, fetch it
if self.object is not None:

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

I generally don't like if ... not tests; I find them slightly hard to read in general. This one happens to be OK, though, although I think I'd very slightly prefer if the early return were in a else.

return self.cleaned_data['buglist']

bugtext = self.cleaned_data['buglist']

# Turn the list into a set to filter out duplicates
buglist = set(BugsForm.url_list_from_bugtext(bugtext))

u = URLValidator()

def is_valid_url(x):

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

It's a bit odd to have this inner function here; since it doesn't require any of the local variables, I'd prefer to see it moved up into a top-level declaration in this module.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 11, 2014

Author Member

It was your idea to put this here! :P

This code also isn't really reusable anywhere else. I suppose refactoring the whole section would still make sense.

try:
u(x)
except django.forms.ValidationError:
return False
return True

if not all([is_valid_url(url) for url in buglist]):

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

Similar remark as above; I have a personal preference against if not. Otherwise, this is great.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 11, 2014

Author Member

Coming from the land of perl, I was really missing me some unless. I'm terrible, I know :D

raise django.forms.ValidationError(
"You have entered an invalid URL: " + url) # evil

# Put this back into text blob format
return "\n".join(buglist)

def save(self):
s = mysite.bugsets.models.BugSet(

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

Does this do the right thing if event_name is the empty string? I'm also unexcited that this may crash if people submit bad form data; do we check if event_name is an existing event, in clean_event_name()? If not, we should.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 11, 2014

Author Member

Event names can collide, so we don't care if it corresponds with an existing event name. (This is why everything we do with sets is explicitly based on the pk.)

If an empty event_name is entered, django happily responds with an error stating the field is required. So it does appear to do the right thing! Added a test to reinforce this idea.

name=self.cleaned_data.get('event_name'))
s.save()

for url in self.cleaned_data.get('buglist').split("\n"):
b = mysite.bugsets.models.AnnotatedBug(url=url)

try:
o = mysite.search.models.Bug.all_bugs.get(
canonical_bug_link=url)

b.title = o.title

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

This is awesome.

b.description = o.description
b.project = o.project

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

I recommend that you steal status info, too! But that can be a later-on thing.

if o.project:
b.skill_list = o.project.language
except ObjectDoesNotExist:
# No problems here, this URL wasn't cached in our main db
pass

b.save()
s.bugs.add(b)

# We need the form to return the set info
self.object = s

def update(self):
s = self.object
old_name = s.name
old_bugs = set(BugsForm.bug_list_from_set(s))

new_name = self.cleaned_data.get('event_name')
new_bugs = set(BugsForm.url_list_from_bugtext(
self.cleaned_data.get('buglist')))

if old_name != new_name:
s.name = new_name
s.save()

for bug in old_bugs - new_bugs:
s.bugs.remove(mysite.bugsets.models.AnnotatedBug.objects.get(
url=bug))

for bug in new_bugs - old_bugs:
# get_or_create returns a tuple (object b, bool created?)
b = mysite.bugsets.models.AnnotatedBug.objects.get_or_create(
url=bug)[0]
s.bugs.add(b)
@@ -0,0 +1,162 @@
# encoding: utf-8
import datetime
from south.db import db
from south.v2 import SchemaMigration
from django.db import models

class Migration(SchemaMigration):

def forwards(self, orm):

# Deleting model 'Skill'
db.delete_table('bugsets_skill')

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

Less is more.


# Removing M2M table for field skills on 'AnnotatedBug'
db.delete_table('bugsets_annotatedbug_skills')


def backwards(self, orm):

# Adding model 'Skill'
db.create_table('bugsets_skill', (
('text', self.gf('django.db.models.fields.CharField')(max_length=200, unique=True)),
('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)),
))
db.send_create_signal('bugsets', ['Skill'])

# Adding M2M table for field skills on 'AnnotatedBug'
db.create_table('bugsets_annotatedbug_skills', (
('id', models.AutoField(verbose_name='ID', primary_key=True, auto_created=True)),
('annotatedbug', models.ForeignKey(orm['bugsets.annotatedbug'], null=False)),
('skill', models.ForeignKey(orm['bugsets.skill'], null=False))
))
db.create_unique('bugsets_annotatedbug_skills', ['annotatedbug_id', 'skill_id'])


models = {
'auth.group': {
'Meta': {'object_name': 'Group'},
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}),
'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'})
},
'auth.permission': {
'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'},
'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'name': ('django.db.models.fields.CharField', [], {'max_length': '50'})
},
'auth.user': {
'Meta': {'object_name': 'User'},
'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2014, 8, 1, 17, 39, 25, 100793)'}),
'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}),
'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(2014, 8, 1, 17, 39, 25, 100740)'}),
'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}),
'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}),
'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}),
'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'})
},
'bugsets.annotatedbug': {
'Meta': {'object_name': 'AnnotatedBug'},
'assigned_to': ('django.db.models.fields.CharField', [], {'max_length': '200', 'blank': 'True'}),
'description': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'mentor': ('django.db.models.fields.CharField', [], {'max_length': '200', 'blank': 'True'}),
'project': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['search.Project']", 'null': 'True', 'blank': 'True'}),
'skill_list': ('django.db.models.fields.CharField', [], {'max_length': '500', 'blank': 'True'}),
'status': ('django.db.models.fields.CharField', [], {'default': "'u'", 'max_length': '1'}),
'time_estimate': ('django.db.models.fields.CharField', [], {'max_length': '200', 'blank': 'True'}),
'title': ('django.db.models.fields.CharField', [], {'max_length': '500', 'blank': 'True'}),
'url': ('django.db.models.fields.URLField', [], {'unique': 'True', 'max_length': '200'})
},
'bugsets.bugset': {
'Meta': {'object_name': 'BugSet'},
'bugs': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['bugsets.AnnotatedBug']", 'symmetrical': 'False'}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'name': ('django.db.models.fields.CharField', [], {'max_length': '200'})
},
'contenttypes.contenttype': {
'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"},
'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
'name': ('django.db.models.fields.CharField', [], {'max_length': '100'})
},
'customs.webresponse': {
'Meta': {'object_name': 'WebResponse'},
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'response_headers': ('django.db.models.fields.TextField', [], {}),
'status': ('django.db.models.fields.IntegerField', [], {}),
'text': ('django.db.models.fields.TextField', [], {}),
'url': ('django.db.models.fields.TextField', [], {})
},
'profile.dataimportattempt': {
'Meta': {'object_name': 'DataImportAttempt'},
'completed': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'date_created': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.utcnow'}),
'failed': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'person': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['profile.Person']"}),
'query': ('django.db.models.fields.CharField', [], {'max_length': '200'}),
'source': ('django.db.models.fields.CharField', [], {'max_length': '2'}),
'web_response': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['customs.WebResponse']", 'null': 'True'})
},
'profile.person': {
'Meta': {'object_name': 'Person'},
'bio': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
'blacklisted_repository_committers': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['profile.RepositoryCommitter']", 'symmetrical': 'False'}),
'contact_blurb': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
'dont_guess_my_location': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'email_me_re_projects': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
'expand_next_steps': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
'gotten_name_from_ohloh': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'homepage_url': ('django.db.models.fields.URLField', [], {'default': "''", 'max_length': '200', 'blank': 'True'}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'irc_nick': ('django.db.models.fields.CharField', [], {'max_length': '30', 'null': 'True', 'blank': 'True'}),
'last_polled': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime(1970, 1, 1, 0, 0)'}),
'latitude': ('django.db.models.fields.FloatField', [], {'default': '-37.3049962'}),
'location_confirmed': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'location_display_name': ('django.db.models.fields.CharField', [], {'default': "'Inaccessible Island'", 'max_length': '255', 'blank': 'True'}),
'longitude': ('django.db.models.fields.FloatField', [], {'default': '-12.6790445'}),
'photo': ('django.db.models.fields.files.ImageField', [], {'default': "''", 'max_length': '100'}),
'photo_thumbnail': ('django.db.models.fields.files.ImageField', [], {'default': "''", 'max_length': '100', 'null': 'True'}),
'photo_thumbnail_20px_wide': ('django.db.models.fields.files.ImageField', [], {'default': "''", 'max_length': '100', 'null': 'True'}),
'photo_thumbnail_30px_wide': ('django.db.models.fields.files.ImageField', [], {'default': "''", 'max_length': '100', 'null': 'True'}),
'show_email': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'unique': 'True'})
},
'profile.repositorycommitter': {
'Meta': {'unique_together': "(('project', 'data_import_attempt'),)", 'object_name': 'RepositoryCommitter'},
'data_import_attempt': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['profile.DataImportAttempt']"}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'project': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['search.Project']"})
},
'search.project': {
'Meta': {'object_name': 'Project'},
'cached_contributor_count': ('django.db.models.fields.IntegerField', [], {'default': '0', 'null': 'True'}),
'created_date': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'null': 'True', 'blank': 'True'}),
'date_icon_was_fetched_from_ohloh': ('django.db.models.fields.DateTimeField', [], {'default': 'None', 'null': 'True'}),
'display_name': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '200'}),
'homepage': ('django.db.models.fields.URLField', [], {'default': "''", 'max_length': '200', 'blank': 'True'}),
'icon_for_profile': ('django.db.models.fields.files.ImageField', [], {'default': 'None', 'max_length': '100', 'null': 'True'}),
'icon_for_search_result': ('django.db.models.fields.files.ImageField', [], {'default': 'None', 'max_length': '100', 'null': 'True'}),
'icon_raw': ('django.db.models.fields.files.ImageField', [], {'default': 'None', 'max_length': '100', 'null': 'True', 'blank': 'True'}),
'icon_smaller_for_badge': ('django.db.models.fields.files.ImageField', [], {'default': 'None', 'max_length': '100', 'null': 'True'}),
'icon_url': ('django.db.models.fields.URLField', [], {'max_length': '200'}),
'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
'language': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '200', 'blank': 'True'}),
'logo_contains_name': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
'modified_date': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}),
'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '200'}),
'people_who_wanna_help': ('django.db.models.fields.related.ManyToManyField', [], {'related_name': "'projects_i_wanna_help'", 'symmetrical': 'False', 'to': "orm['profile.Person']"})
}
}

complete_apps = ['bugsets']
@@ -30,13 +30,6 @@
# }}}


class Skill(models.Model):
text = models.CharField(max_length=200, unique=True)

def __unicode__(self):
return self.text


class AnnotatedBug(models.Model):
url = models.URLField(max_length=200, unique=True)
title = models.CharField(max_length=500, blank=True)
@@ -55,9 +48,6 @@ class AnnotatedBug(models.Model):
status = models.CharField(max_length=1, default='u',
choices=STATUS_CHOICES)

# Putting this on hold for the MVP
skills = models.ManyToManyField(Skill)

# Kludgey replacement field for skills
skill_list = models.CharField(max_length=500, blank=True)

@@ -77,3 +67,9 @@ def get_absolute_url(self):
'pk': self.pk,
'slug': slugify(self.name),
})

def get_edit_url(self):
return reverse(mysite.bugsets.views.create_index, kwargs={

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

Sweet!

'pk': self.pk,
'slug': slugify(self.name),
})
@@ -20,6 +20,7 @@

from django.conf import settings


class InlineEditPermissions(object):

@classmethod
@@ -1,4 +1,4 @@
{% extends 'base/base.html' %}
{% extends 'base/one_column.html' %}

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

Yay! Glad to see this get more consistent.

{% comment %}
# This file is part of OpenHatch.
# Copyright (C) 2014 Elana Hashman
@@ -21,24 +21,25 @@
Create Bug Set
{% endblock title %}


{% block body_id %}bugset_create{% endblock %}

{% block pagetop %}
<h1 style='margin: 10px 0 0 10px; font-weight: bold; color: #222'>
Create a Bug Set
</h1>
{% endblock %}

{% block content %}
{% block main %}
<div class='module'>
<div class='head'>
<h3>{% block heading %}Header{% endblock %}</h3>
<p>
Here is some placeholder text.
</p>
</div>
<div class='body'>
{% for set in bugsets.all %}
<p><a href="{{ set.get_absolute_url }}">{{ set.name }}</a></p>
{% endfor %}
<div class='module-body'>
<form action="{% url "mysite.bugsets.views.create_index" %}" method="post">

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 7, 2014

Contributor

This is inconsistent with the OpenHatch idiom of having a _do view for receiving a POST, but you have chosen the more Django-y way to do this, so who can blame you.

{% csrf_token %}
{{ form.as_p }}
<div id="button-container">
<input type="submit" value="OK">
</div>
</form>
</div>
</div>
{% endblock content %}
{% endblock main %}
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.