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

Allow users to swap project IDs across timecard entries #886

Merged
merged 4 commits into from
Jan 24, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions tock/hours/forms.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
import json

import bleach

from django import forms
from django.forms.models import BaseInlineFormSet
from django.forms.models import inlineformset_factory
from django.utils.html import escapejs
from django.db import connection, transaction
from django.db.models import Prefetch
from django.forms.models import BaseInlineFormSet, inlineformset_factory
from django.utils.html import escapejs

from .models import Timecard, TimecardObject, ReportingPeriod
from projects.models import AccountingCode, Project

from .models import ReportingPeriod, Timecard, TimecardObject


class ReportingPeriodForm(forms.ModelForm):
"""Form for creating new reporting periods"""

Expand Down Expand Up @@ -162,7 +163,6 @@ def clean_hours_spent(self):
return self.cleaned_data.get('hours_spent') or 0

def clean(self):
super(TimecardObjectForm, self).clean()
if 'notes' in self.cleaned_data and 'project' in self.cleaned_data:
self.cleaned_data['notes'] = bleach.clean(
self.cleaned_data['notes'],
Expand Down Expand Up @@ -261,6 +261,17 @@ def clean(self):

return getattr(self, 'cleaned_data', None)

def save(self, commit=True):
"""
Save with deferred constraints
Allowing users to swap project IDs between TimeCardObjects
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this gets a bit down into the gears of things, can we either write up more on what we're doing or is there a link we can offer that gets into the details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrote this up more comprehensively in #887

Copy link
Contributor

@tbaxter-18f tbaxter-18f Jan 24, 2019

Choose a reason for hiding this comment

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

Thats great, but I'm thinking of future person reading forms.py who's confused by what this is and why we did it. I want to leave them a bit more context here (or link to more context). I'd be totally OK just linking back to #887 for future person, though. That's a pretty in-depth explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! Added details clarifying the custom save behavior here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great. Thank you!

cur = connection.cursor()
with transaction.atomic():
cur.execute("SET CONSTRAINTS ALL DEFERRED")
formset = super().save(commit=commit)
return formset


def timecard_formset_factory(extra=1):
return inlineformset_factory(
Expand Down
28 changes: 28 additions & 0 deletions tock/hours/migrations/0045_deferrable_unique.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Generated by Django 2.0.9 on 2019-01-17 18:28

from django.db import migrations


class Migration(migrations.Migration):
"""
UNIQUE constraints are created by Django with the postgres default of NONDEFFERABLE
Recreating TimeCardObject's UNIQUE constraint as DEFERRABLE
"""
dependencies = [
('hours', '0044_delete_targets'),
]

DROP_UNIQUE_CONSTRAINT = """
ALTER TABLE public.hours_timecardobject
DROP CONSTRAINT hours_timecardobject_timecard_id_project_id_b3d0a465_uniq
"""
ADD_NON_DEFERRABLE = """
ALTER TABLE public.hours_timecardobject
ADD CONSTRAINT hours_timecardobject_timecard_id_project_id_b3d0a465_uniq
UNIQUE (project_id, timecard_id)
"""
ADD_DEFERRABLE = ADD_NON_DEFERRABLE + " DEFERRABLE"

operations = [migrations.RunSQL(DROP_UNIQUE_CONSTRAINT, ADD_NON_DEFERRABLE),
migrations.RunSQL(ADD_DEFERRABLE, DROP_UNIQUE_CONSTRAINT)
]
15 changes: 15 additions & 0 deletions tock/hours/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,21 @@ def save(self, *args, **kwargs):
super(TimecardObject, self).save(*args, **kwargs)
Jkrzy marked this conversation as resolved.
Show resolved Hide resolved


def to_csv_row(self):
"""Output attributes for csv.writer consumption"""
return [
"{0} - {1}".format(
self.timecard.reporting_period.start_date,
self.timecard.reporting_period.end_date
),
self.timecard.modified.strftime("%Y-%m-%d %H:%M:%S"),
self.timecard.user.username,
self.project,
self.hours_spent,
self.timecard.user.user_data.organization_name,
self.project.organization_name
]

class TimecardPrefillDataManager(models.Manager):
def active(self):
return super(TimecardPrefillDataManager, self).get_queryset().filter(
Expand Down
83 changes: 59 additions & 24 deletions tock/hours/tests/test_forms.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import datetime

from django.test import TestCase
from django.test import TestCase, TransactionTestCase
from django.contrib.auth import get_user_model

import hours.models
Expand Down Expand Up @@ -116,42 +116,77 @@ def test_general_notes_field_strips_html(self):
self.assertTrue(form.is_valid())
self.assertEqual(form.cleaned_data['notes'], 'This is a test!')

class TimecardInlineFormSetTests(TestCase):
fixtures = [
'projects/fixtures/projects.json', 'tock/fixtures/prod_user.json']

def setUp(self):
self.reporting_period = hours.models.ReportingPeriod.objects.create(
start_date=datetime.date(2015, 1, 1),
def time_card_inlineformset_setup(self):
self.reporting_period = hours.models.ReportingPeriod.objects.create(
start_date=datetime.date(2015, 1, 1),
end_date=datetime.date(2015, 1, 7),
exact_working_hours=40,
min_working_hours=40,
max_working_hours=60
)
self.user = get_user_model().objects.get(id=1)
self.project_1 = projects.models.Project.objects.get(name="openFEC")
self.project_2 = projects.models.Project.objects.get(
name="Peace Corps")
self.project_3 = projects.models.Project.objects.get(name='General')
self.project_3.notes_displayed = True
self.project_3.notes_required = True
self.project_3.save()
self.timecard = hours.models.Timecard.objects.create(
reporting_period=self.reporting_period,
)
self.user = get_user_model().objects.get(id=1)
self.project_1 = projects.models.Project.objects.get(name="openFEC")
self.project_2 = projects.models.Project.objects.get(
name="Peace Corps")
self.project_3 = projects.models.Project.objects.get(name='General')
self.project_3.notes_displayed = True
self.project_3.notes_required = True
self.project_3.save()
self.timecard = hours.models.Timecard.objects.create(
reporting_period=self.reporting_period,
user=self.user)

def form_data(self, clear=[], **kwargs):
""" Method that auto generates form data for tests """
form_data = {
'timecardobjects-TOTAL_FORMS': '2',
self.initial_form_data = {
'timecardobjects-TOTAL_FORMS': '2',
'timecardobjects-INITIAL_FORMS': '0',
'timecardobjects-MIN_NUM_FORMS': '',
'timecardobjects-MAX_NUM_FORMS': '',
'timecardobjects-0-project': '4',
'timecardobjects-0-hours_spent': '22',
'timecardobjects-0-hours_spent': '20',
'timecardobjects-1-project': '5',
'timecardobjects-1-hours_spent': '20'
}
}


class TimecardInlineFormSetTransactionTests(TransactionTestCase):
fixtures = [
'projects/fixtures/projects.json', 'tock/fixtures/prod_user.json',
'employees/fixtures/user_data.json'
]

setUp = time_card_inlineformset_setup

def test_timecard_inline_formset_modify_saved(self):
"""Users can swap project IDs between TimeCardObjects """
form_data = self.initial_form_data
formset = TimecardFormSet(form_data, instance=self.timecard)
# Save these timecard entries for later modification
formset.save_only = True
formset.is_valid()
formset.save()

# We've got a saved timecard, lets try to edit it by swapping the projects
project5 = self.timecard.timecardobjects.get(project_id=5)
project4 = self.timecard.timecardobjects.get(project_id=4)
form_data.update({'timecardobjects-0-id': project4.id,
'timecardobjects-1-id': project5.id,
'timecardobjects-0-project': '5',
'timecardobjects-1-project': '4',
'timecardobjects-INITIAL_FORMS': '2'})
formset = TimecardFormSet(form_data, instance=self.timecard)
formset.is_valid()
formset.save()

class TimecardInlineFormSetTests(TestCase):
fixtures = [
'projects/fixtures/projects.json', 'tock/fixtures/prod_user.json']

setUp = time_card_inlineformset_setup

def form_data(self, clear=[], **kwargs):
""" Method that auto generates form data for tests """
form_data = self.initial_form_data
for key in clear:
del form_data[key]
for key, value in kwargs.items():
Expand Down
20 changes: 12 additions & 8 deletions tock/hours/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -716,22 +716,26 @@ def test_create_reporting_period_not_superuser(self):
self.assertEqual(response.status_code, 403)

def test_ReportingPeriodCSVView(self):
"""
CSV row representation of included TimeCardObjects
must be present in generated CSV
"""
reporting_period = '2015-01-01'
expected = hours.models.TimecardObject.objects.filter(
timecard__reporting_period__start_date=reporting_period
).first()
response = self.app.get(
reverse(
'reports:ReportingPeriodCSVView',
kwargs={'reporting_period': '2015-01-01'},
kwargs={'reporting_period': reporting_period},
),
user=self.regular_user
)
lines = response.content.decode('utf-8').splitlines()
self.assertEqual(len(lines), 3)
result = '2015-01-01 - 2015-01-07,{0},aaron.snow,Peace Corps,28.00,,'
self.assertEqual(
result.format(
self.timecard.modified.strftime('%Y-%m-%d %H:%M:%S')
),
lines[2],
)
# coerce to comma delimited string for comparison
expected_csv_row = ','.join([str(x) for x in expected.to_csv_row()])
self.assertIn(expected_csv_row, lines)

def test_ReportingPeriodCSVView_add_additional_row(self):
"""
Expand Down
13 changes: 1 addition & 12 deletions tock/hours/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -770,18 +770,7 @@ def ReportingPeriodCSVView(request, reporting_period):
if not timecard_object.timecard.submitted:
continue

writer.writerow([
"{0} - {1}".format(
timecard_object.timecard.reporting_period.start_date,
timecard_object.timecard.reporting_period.end_date
),
timecard_object.timecard.modified.strftime("%Y-%m-%d %H:%M:%S"),
timecard_object.timecard.user.username,
timecard_object.project,
timecard_object.hours_spent,
timecard_object.timecard.user.user_data.organization_name,
timecard_object.project.organization_name
])
writer.writerow(timecard_object.to_csv_row())

return response

Expand Down