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

Conversation

Jkrzy
Copy link
Contributor

@Jkrzy Jkrzy commented Jan 23, 2019

Recreating the TimeCardObject unique constraint as deferrable and deferring when saving TimeCardObject formsets thereby allowing users to swap project IDs between
existing entries on a timecard.

Description

Prior to this PR, attempts to swap project IDs between existing entries, either intentional or inadvertent, prevented users from submitting their timecard without providing any useful error message or pathway to resolve.

Deferring constraints when saving TimeCardObject formsets
thereby allowing users to swap project IDs between
existing entries on a timecard.
@Jkrzy Jkrzy changed the title [WIP] Allow users to swap project IDs across timecard entries Allow users to swap project IDs across timecard entries Jan 23, 2019
"""
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!

@tbaxter-18f
Copy link
Contributor

This resolves #881 right?

@tbaxter-18f tbaxter-18f reopened this Jan 23, 2019
@Jkrzy
Copy link
Contributor Author

Jkrzy commented Jan 23, 2019

Thanks @tbaxter-18f, We'll address #881 separately, this PR doesn't include any modification/assessment of TimeCardOjbect.submitted

@codecov-io
Copy link

codecov-io commented Jan 24, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #886      +/-   ##
==========================================
+ Coverage    91.6%   91.64%   +0.03%     
==========================================
  Files          39       39              
  Lines        1680     1687       +7     
==========================================
+ Hits         1539     1546       +7     
  Misses        141      141
Impacted Files Coverage Δ
hours/views.py 88.23% <0%> (ø) ⬆️
hours/models.py 96.25% <0%> (+0.04%) ⬆️
hours/forms.py 94.61% <0%> (+0.21%) ⬆️

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 51f109e...b096f40. Read the comment docs.

@Jkrzy
Copy link
Contributor Author

Jkrzy commented Jan 24, 2019

Working towards a resolution of #887 with this PR.

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.

3 participants