-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix: set status for canceled cases #1202
Conversation
f80e2cb
to
cadf6fd
Compare
case=None, | ||
**kwargs, | ||
): | ||
def set_case_status(case=None, work_item=None): |
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.
This function has so little common code, I'd just split it in two and get rid of the whole if workitem else
stuff
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.
Agreed and done 👍
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.
This messes up my precious coverage 😖
cadf6fd
to
38ab62d
Compare
ffd4878
to
9c1fec8
Compare
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.
LGTM
@winged Had to refactor it a little more. But now it's also more better ;) Additionally I saw, Caluma uses the spelling |
9c1fec8
to
b20ebe2
Compare
Nice! BTW why are we using a meta field here instead of referring to the case's own status field? |
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.
LGTM, just a small nitpick I've found. Feel free to ignore tho
work_item = ( | ||
work_item if work_item else case.work_items.filter(status="ready").first() | ||
) | ||
work_item = case.work_items.filter(status="ready").first() |
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.
I'd probably try to use a constant here
work_item = case.work_items.filter(status="ready").first() | |
work_item = case.work_items.filter(status=WorkItem.STATUS_READY).first() |
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.
I'm all here for the nitpicks ;)
You are correct and it's fixed 👍
Cancelled cases are still accessible via their URL. So the status needs to be set correctly. This commit adds a new status `canceled` and sets it on cancelation of a case. Additionally the event handlers were refactored in the following way: - only cases with the `document-review` Workflow get a status applied. - The status is set on every change of ready WorkItems, not only on the expected forward changes. This makes sure, that the status is also correct, if a Case was closed and then reopened.
b20ebe2
to
457b06b
Compare
Cancelled cases are still accessible via their URL. So the status needs
to be set correctly. This commit adds a new status
canceled
andsets it on cancelation of a case.
Additionally the event handlers were refactored in the following way:
document-review
Workflow get a status applied.expected forward changes. This makes sure, that the status is also
correct, if a Case was closed and then reopened.
drive-by commit