-
Notifications
You must be signed in to change notification settings - Fork 18
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
Generic javascript template support [WIP] #8
base: master
Are you sure you want to change the base?
Changes from 8 commits
ee32c69
db68e1e
f9dec35
045a280
957db07
e855286
693cff3
12ad933
dc08649
97f0259
bd398c1
3da6d64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,13 @@ def __unicode__(self): | |
class Meta: | ||
abstract = True | ||
|
||
@staticmethod | ||
def to_json(task_group): | ||
return { | ||
'group_id': task_group.group_id, | ||
'tasks_finished': task_group.tasks_finished, | ||
} | ||
|
||
|
||
# Model for an individual task. | ||
class AbstractCrowdTask(models.Model): | ||
|
@@ -68,6 +75,20 @@ def __unicode__(self): | |
class Meta: | ||
abstract = True | ||
|
||
@staticmethod | ||
def to_json(task): | ||
return { | ||
'task_type': task.task_type, | ||
'data': task.data, | ||
'creation_time': task.create_time, | ||
'task_id': task.task_id, | ||
'num_assignments': task.num_assignments, | ||
'mv_answer': task.mv_answer, | ||
'em_answer': task.em_answer, | ||
'is_complete': task.is_complete, | ||
'completed_assignments': len(task.responses.all()) | ||
} | ||
|
||
|
||
# Model for workers | ||
class AbstractCrowdWorker(models.Model): | ||
|
@@ -150,3 +171,43 @@ def add_model_rels(self): | |
# responses pertain to a task | ||
self.add_rel(self.response_model, self.task_model, models.ForeignKey, | ||
'task', 'responses') | ||
|
||
|
||
class TemplateResource(models.Model): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good if these models lived in a different file from the abstract models that should be subclassed to implement new crowds. It might make sense to restructure into a |
||
name = models.CharField(max_length=200) | ||
content = models.TextField() | ||
direct_dependencies = models.ManyToManyField('self', symmetrical=False, related_name="upstream_dependencies") | ||
direct_requirements = models.ManyToManyField('self', symmetrical=False, related_name="upstream_requirements") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick, but if I'm interpreting this right, shouldn't the related_name be Also remind me again the difference between dependencies and requirements? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The names I chose for these is very poor. The idea is that a template might have
For example, suppose I have two js templates. In order for one to work, it requires the other. This would be one way to enforce this and have a way to show which templates are related to each other. The other case would be a way to include stuff like jquery/react within the system instead within the template. Upon further thought, I think it would help if I renamed those to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still not totally understanding. Why would an import not also be a requirement? Are you trying to support the use case where templates gracefully degrade if some (non-required) imports are not available? Or are you trying to differentiate between template inheritance and including external libraries? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second one is closer to the intent. Dependencies (or imports) would be akin to import statements in python, or Requirements are for enforcing template inheritance/composition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, then But I'm wondering if it wouldn't be cleaner to place these in separate models, one model for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that idea, it cleans things up a bit and makes it clearer. This week is a bit busy at work, but hopefully I can find some time to work on it on evenings now that I finished a refactor on another project. |
||
|
||
@property | ||
def dependencies(self): | ||
query_set = list(self.direct_dependencies.all()) | ||
all_dependencies = set(query_set) | ||
current_dependencies = list(query_set) | ||
while True: | ||
prior_size = len(all_dependencies) | ||
new_dependencies = set() | ||
for dep in current_dependencies: | ||
deps = set(dep.direct_dependencies.all()) | ||
new_dependencies = new_dependencies.union(deps) | ||
all_dependencies = all_dependencies.union(deps) | ||
if prior_size == len(all_dependencies): | ||
break | ||
else: | ||
current_dependencies = list(new_dependencies) | ||
return all_dependencies | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks pretty painfully inefficient in terms of DB queries. I'll put a little thought into it, but there should be an easier way to do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. |
||
@property | ||
def requirements(self): | ||
pass | ||
|
||
|
||
def __str__(self): | ||
return self.name | ||
|
||
|
||
class TaskType(models.Model): | ||
name = models.CharField(max_length=200) | ||
iterator_template = models.ForeignKey(TemplateResource, related_name='iterator_task') | ||
point_template = models.ForeignKey(TemplateResource, related_name='point_task') | ||
renderer = models.ForeignKey(TemplateResource, related_name='renderer_task') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
<!DOCTYPE html> | ||
<html> | ||
<head lang="en"> | ||
<meta charset="UTF-8"> | ||
<title></title> | ||
</head> | ||
<body> | ||
|
||
</body> | ||
</html> |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
from django.test import TestCase | ||
from basecrowd.models import TaskType, TemplateResource | ||
|
||
|
||
class TasksTestCase(TestCase): | ||
def setUp(self): | ||
html = TemplateResource.objects.create(name="html") | ||
js = TemplateResource.objects.create(name="js") | ||
css = TemplateResource.objects.create(name="css") | ||
bootstrap = TemplateResource.objects.create(name="bootstrap") | ||
bootstrap.direct_dependencies.add(html, js, css) | ||
|
||
jquery = TemplateResource.objects.create(name="jquery") | ||
jquery.direct_dependencies.add(html, js, css) | ||
|
||
reactjs = TemplateResource.objects.create(name="reactjs") | ||
reactjs.direct_dependencies.add(js) | ||
|
||
jqueryui = TemplateResource.objects.create(name="jqueryui") | ||
jqueryui.direct_dependencies.add(jquery) | ||
|
||
bootstrap_react_gallery = TemplateResource.objects.create(name="bootstrap react gallery") | ||
bootstrap_react_gallery.direct_dependencies.add(reactjs, bootstrap) | ||
|
||
monolithic_app = TemplateResource.objects.create(name="monolithic app") | ||
monolithic_app.direct_dependencies.add( | ||
bootstrap_react_gallery, jqueryui, jquery, js, css, html, reactjs | ||
) | ||
|
||
def test_template_dependencies(self): | ||
html = TemplateResource.objects.get(name="html") | ||
css = TemplateResource.objects.get(name="css") | ||
js = TemplateResource.objects.get(name="js") | ||
jquery = TemplateResource.objects.get(name="jquery") | ||
bootstrap = TemplateResource.objects.get(name="bootstrap") | ||
reactjs = TemplateResource.objects.get(name="reactjs") | ||
jqueryui = TemplateResource.objects.get(name="jqueryui") | ||
bootstrap_react_gallery = TemplateResource.objects.get(name="bootstrap react gallery") | ||
monolithic_app = TemplateResource.objects.get(name="monolithic app") | ||
self.assertSetEqual( | ||
jquery.dependencies, | ||
{html, css, js} | ||
) | ||
self.assertSetEqual( | ||
html.dependencies, | ||
set() | ||
) | ||
self.assertSetEqual( | ||
reactjs.dependencies, | ||
{js} | ||
) | ||
self.assertSetEqual( | ||
jqueryui.dependencies, | ||
{jquery, html, js, css} | ||
) | ||
self.assertSetEqual( | ||
bootstrap_react_gallery.dependencies, | ||
{html, js, css, reactjs, bootstrap} | ||
) | ||
self.assertSetEqual( | ||
monolithic_app.dependencies, | ||
{html, js, css, reactjs, jquery, jqueryui, bootstrap, bootstrap_react_gallery} | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,4 +8,6 @@ | |
url(r'^(\w+)/responses/$', views.post_response, name='post_response'), | ||
url(r'^(\w+)/tasks/$', views.create_task_group, name='create_tasks'), | ||
url(r'^(\w+)/purge_tasks/$', views.purge_tasks, name='purge_tasks'), | ||
url(r'^(\w+)/summary/task_groups', views.get_task_groups, name='get_task_groups'), | ||
url(r'^(\w+)/summary/tasks', views.get_tasks, name="get_tasks") | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is related to your demo dashboard, not the template stuff, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, its dashboard. Would it be better to make that a different pull request, or include that? Those are basically poorly urled, json endpoints for tasks and task groups. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's too much hassle, I'm fine keeping it as one PR. But then we could definitely clean up the urls 😄 I'm not sure why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
/* | ||
* Base structure | ||
*/ | ||
|
||
/* Move down content because we have a fixed navbar that is 50px tall */ | ||
body { | ||
padding-top: 50px; | ||
} | ||
|
||
|
||
/* | ||
* Global add-ons | ||
*/ | ||
|
||
.sub-header { | ||
padding-bottom: 10px; | ||
border-bottom: 1px solid #eee; | ||
} | ||
|
||
/* | ||
* Top navigation | ||
* Hide default border to remove 1px line. | ||
*/ | ||
.navbar-fixed-top { | ||
border: 0; | ||
} | ||
|
||
/* | ||
* Sidebar | ||
*/ | ||
|
||
/* Hide for mobile, show later */ | ||
.sidebar { | ||
display: none; | ||
} | ||
@media (min-width: 768px) { | ||
.sidebar { | ||
position: fixed; | ||
top: 51px; | ||
bottom: 0; | ||
left: 0; | ||
z-index: 1000; | ||
display: block; | ||
padding: 20px; | ||
overflow-x: hidden; | ||
overflow-y: auto; /* Scrollable contents if viewport is shorter than content. */ | ||
background-color: #f5f5f5; | ||
border-right: 1px solid #eee; | ||
} | ||
} | ||
|
||
/* Sidebar navigation */ | ||
.nav-sidebar { | ||
margin-right: -21px; /* 20px padding + 1px border */ | ||
margin-bottom: 20px; | ||
margin-left: -20px; | ||
} | ||
.nav-sidebar > li > a { | ||
padding-right: 20px; | ||
padding-left: 20px; | ||
} | ||
.nav-sidebar > .active > a, | ||
.nav-sidebar > .active > a:hover, | ||
.nav-sidebar > .active > a:focus { | ||
color: #fff; | ||
background-color: #428bca; | ||
} | ||
|
||
|
||
/* | ||
* Main content | ||
*/ | ||
|
||
.main { | ||
padding: 20px; | ||
} | ||
@media (min-width: 768px) { | ||
.main { | ||
padding-right: 40px; | ||
padding-left: 40px; | ||
} | ||
} | ||
.main .page-header { | ||
margin-top: 0; | ||
} | ||
|
||
|
||
/* | ||
* Placeholder dashboard ideas | ||
*/ | ||
|
||
.placeholders { | ||
margin-bottom: 30px; | ||
text-align: center; | ||
} | ||
.placeholders h4 { | ||
margin-bottom: 0; | ||
} | ||
.placeholder { | ||
margin-bottom: 20px; | ||
} | ||
.placeholder img { | ||
display: inline-block; | ||
border-radius: 50%; | ||
} |
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.
these methods aren't actually outputting JSON. Either have the method return
json.dumps(return_dict)
, or rename itto_dict
().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 catch, the
json_view
decorator can automatically convert the dict so will doto_dict