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

Convert business units to DB table #992

Merged
merged 6 commits into from
Mar 11, 2020
Merged

Convert business units to DB table #992

merged 6 commits into from
Mar 11, 2020

Conversation

tbaxter-18f
Copy link
Contributor

Description

Resolves #965 and converts business units to a DB table instead of choices, along with updated tests and fixtures. Also cleans up some tech debt around how orgs are defined and used.

@tbaxter-18f tbaxter-18f changed the title Migrated, tested and 'done' Convert business units to DB table Mar 10, 2020
@@ -94,21 +94,13 @@ class GeneralSnippetsTimecardSerializer(serializers.Serializer):
)
hours_spent = serializers.DecimalField(max_digits=5, decimal_places=2)
notes = serializers.CharField()
unit = serializers.SerializerMethodField()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had already defined unit on L88

@@ -65,7 +65,7 @@ class UserDataAdmin(admin.ModelAdmin):
search_fields = ('user__username',)

def unit_info(self, obj):
return obj.get_unit_display()
return obj.unit.name
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to check for None object. It is currently breaking the admin interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch Amy! Updated, and added a comment.

@codecov-io
Copy link

Codecov Report

Merging #992 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
+ Coverage   91.52%   91.60%   +0.07%     
==========================================
  Files          39       39              
  Lines        1699     1691       -8     
==========================================
- Hits         1555     1549       -6     
+ Misses        144      142       -2     
Impacted Files Coverage Δ
employees/admin.py 86.11% <0.00%> (ø)
employees/models.py 100.00% <0.00%> (ø)
hours/views.py 88.58% <0.00%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50fefc1...76fa570. Read the comment docs.

Copy link
Contributor

@amymok amymok left a comment

Choose a reason for hiding this comment

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

LGTM!

@amymok
Copy link
Contributor

amymok commented Mar 11, 2020

Merging this PR, something is not quite working in Snyk. There's no manifest changes.

@amymok amymok merged commit b6ddb1a into master Mar 11, 2020
@Jkrzy Jkrzy deleted the org-table-2 branch May 29, 2020 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert business units to DB table
4 participants