-
Notifications
You must be signed in to change notification settings - Fork 35
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
[SDESK-7444] - Planning: Migrate planning:flag_expired command to async #2149
Conversation
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.
Looks good, just some unit tests failing that needs to be looked into
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.
It looks good, I just left a few comments. I can see these functions rely much on dictionaries. In general, we should lean into using the pydantic models as much as possible.
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.
Looks good to me!
@BrianMwangi21 there are some broken references to old
|
b5508e5
to
5abc0d8
Compare
"planning", | ||
{ | ||
"p1": False, | ||
"p2": False, | ||
"p3": False, | ||
"p4": False, | ||
"p5": True, | ||
"p6": False, | ||
"p6": True, |
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 wondering, why were p6
and p8
changed in the tests from False
to True
. Is there any logic or query that is not the same as the old code?
self.assertExpired("planning", {"p1": False, "p2": False, "p3": False, "p4": True}) | ||
await flag_expired_items_handler() | ||
await self.assertExpired("events", {"e1": False, "e2": False, "e3": False, "e4": True}) | ||
await self.assertExpired("planning", {"p1": False, "p2": False, "p3": True, "p4": True}) |
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.
Also here as well, why is p3
now True
Looks good @BrianMwangi21 👍 |
@BrianMwangi21 Looks like the changes to the test data is required, as the new async Planning service doesn't yet have all the functionality from the original service (which involved processing the Coverages and dates etc), which would be why these tests were failing before your test condition changes. We can look at this later once the async Planning service is updated |
@MarkLark86 this is noted. I've added a TODO on the test file on the same. |
Purpose
This PR updates the
planning:flag_expired command
command to async by:Solves SDESK-7444