-
Notifications
You must be signed in to change notification settings - Fork 1
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
AB#241235: add ! push data to HOPE core #45
Conversation
7b6430d
to
2c2f794
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #45 +/- ##
===========================================
+ Coverage 91.18% 92.40% +1.22%
===========================================
Files 151 152 +1
Lines 4320 4466 +146
Branches 369 389 +20
===========================================
+ Hits 3939 4127 +188
+ Misses 283 236 -47
- Partials 98 103 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c2e94f7
to
f614929
Compare
@vitali-yanushchyk-valor pls resolve conflicts |
f614929
to
1f1aa31
Compare
|
||
def __post_init__(self) -> None: | ||
"""Initialize the base path for API requests.""" | ||
self.base_path = f"{self.co_slug}/rdi/" |
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.
why not using init?
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's dataclass
"""Initialize the base path for API requests.""" | ||
self.base_path = f"{self.co_slug}/rdi/" | ||
|
||
def validate_households(self) -> 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.
shouldn't we have this in another place and call it? looks to me duplicated code
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 only validates the selected beneficiaries, without updating last_checked and add a new version.
""" | ||
try: | ||
with transaction.atomic(): | ||
households = list(CountryHousehold.objects.filter(id__in=successful_ids)) |
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.
why converting it in list?
|
||
""" | ||
processor = PushProcessor( | ||
queryset=CountryHousehold.objects.filter(pk__in=job.config["pks"]), |
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.
would we have problems with large datasets?
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.
Fixed it.
# NEVER CHANGE THIS VALUES | ||
HOUSEHOLD_CHECKER_NAME = "HOPE Household core" | ||
INDIVIDUAL_CHECKER_NAME = "HOPE Individual core" | ||
HOUSEHOLD_CHECKER_NAME: Final[str] = "HOPE Household checker" |
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.
core checker & also for individaul
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.
51f2d42
to
e38ae85
Compare
@vitali-yanushchyk-valor fix tests pls |
e38ae85
to
edee7b1
Compare
# NEVER CHANGE THIS VALUES | ||
HOUSEHOLD_CHECKER_NAME = "HOPE Household core" | ||
INDIVIDUAL_CHECKER_NAME = "HOPE Individual core" | ||
HOUSEHOLD_CHECKER_NAME: Final[str] = "HOPE Household checker" |
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.
hh_fs.fields.get_or_create(name="consent", attrs={"required": False, "label": "Consent"}, definition=_bool) | ||
hh_fs.fields.get_or_create(name="consent_sharing", definition=_bool) | ||
hh_fs.fields.get_or_create(name="country_origin", definition=_char) | ||
hh_fs.fields.get_or_create(name="first_registration_date", attrs={"label": "First Registration"}, definition=_date) |
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.
why we removed this?
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.
datachecker changes follow the current HOPE core.
- consent_sharing - is not bool - it's ConsentSharingEnum and when I asked about it at the sync, I got an answer, that we will not use it.
- country_origin - I declared to use CountryChoice
- first_registration_date at the HOPE core is defined like serializers.DateTimeField(default=timezone.now)
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 updated the system to require full_name, birth_date, and relationship too, since HOPE core will respond with an error if they are not provided.
This is one of the questions I wanted to clarify. We have a base fieldset, and in CW we can create additional fieldsets that extend existing. Since this is the very first commit regarding data push to HOPE core —and we don't yet have such a process in production—I modified the default datachecker in accordance with the current requirements from HOPE core. In the future, this mechanism for creating new fieldsets can be utilized.
Of course, if the base datachecker is meant to remain exactly as it was before my changes, I can revert them.
_h_residence = FieldDefinition.objects.get(slug="hope-hh-residencestatus") | ||
_i_gender = FieldDefinition.objects.get(slug="hope-ind-gender") | ||
_i_disability = FieldDefinition.objects.get(slug="hope-ind-disability") | ||
_i_role = FieldDefinition.objects.get(slug="hope-ind-role") | ||
_i_relationship = FieldDefinition.objects.get(slug="hope-ind-relationship") | ||
|
||
hh_fs, __ = Fieldset.objects.get_or_create(name=HOUSEHOLD_CHECKER_NAME) | ||
hh_fs.fields.get_or_create(name="address", attrs={"label": "Address", "required": True}, definition=_char) | ||
hh_fs.fields.get_or_create(name="address", definition=_char) |
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.
why was this changed?
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.
because currently address in not mandatory at the HOPE core side.
Legal Boilerplate
Look, I get it.
Contributing to HOPE Workspace, I retain all rights, title and interest in and to my contributions, and by keeping
this boilerplate intact I confirm that HOPE Workspace can use, modify, copy, and redistribute my contributions,
under HOPE's choice of terms.