CM-69: Develop logic for handling custom filters for benefit plan - BE #6
Conversation
sniedzielski
requested review from
delcroip,
dborowiecki,
malinowskikam and
jdolkowski
|
@dborowiecki You can start checking PR |
social_protection/custom_filters.py
Outdated
| :return: A list of named tuples representing the definition of how to create filters. | ||
| :rtype: List[namedtuple] | ||
| """ | ||
| logger.info('starting loading definition of custom filter') |
There was a problem hiding this comment.
Is this definition loaded multuple times or only during setup? If the former then this kind of log may bloat logger and it's not essential, you could change it to to debug or simply remove it. I don't see value other then information that function was invoked.
There was a problem hiding this comment.
It could be loaded multiple times. I think this log is a redundant one.
social_protection/custom_filters.py
Outdated
| if benefit_plan: | ||
| self.__process_schema_and_build_tuple(benefit_plan, tuple_type, list_of_tuple_with_definitions) | ||
| else: | ||
| logger.info('There is no benefit plan with such id') |
There was a problem hiding this comment.
I don't think this should be a info. Rather a warning, also if you received invalid ID it's perfectly fine to just throw exeption in here. Returning empty list might be misleading as in fact we have filters definitions, but input data is invalid.
There was a problem hiding this comment.
Changed to the using get() in QuerySet
social_protection/custom_filters.py
Outdated
| else: | ||
| logger.info('There is no benefit plan with such id') | ||
| else: | ||
| logger.info('Id of benefit plan is not provided') |
There was a problem hiding this comment.
Same here, it's not really information log. Information log is used to (among others) to track important events or milestones in the system's execution. They should be used sparingly and contain essential information to avoid cluttering the log files.
social_protection/custom_filters.py
Outdated
| logger.info('There is no benefit plan with such id') | ||
| else: | ||
| logger.info('Id of benefit plan is not provided') | ||
| logger.info('Output is prepared with definition how to build filters') |
There was a problem hiding this comment.
This also can be removed
social_protection/custom_filters.py
Outdated
| else: | ||
| logger.info('There are no properties provided in schema from that benefit plan') | ||
| else: | ||
| logger.info('There is no schema provided in benefit plan') |
There was a problem hiding this comment.
Those logs could be warning instead of info. While I believe that this shouldn't raise exeption (as benefit plan exists), warning could be added. It's because empty schema is possible in the system (at least now), however we cannot use it for custom filters and it should be changed (therefore warning for the developer that will look into an issue with empty custom filters).
social_protection/custom_filters.py
Outdated
| filter=self.FILTERS_BASED_ON_FIELD_TYPE[property['type']], | ||
| type=property['type'] | ||
| ) | ||
| list_of_tuple_with_definitions.append(tuple_with_definition) |
There was a problem hiding this comment.
This is similar case to the one in core module. I think it'll be bettet to simply return new defintiions instead of appending them to the list from outside context.
social_protection/custom_filters.py
Outdated
| if schema: | ||
| if 'properties' in schema: |
There was a problem hiding this comment.
When log (see next comments) will be changed, this can also be simplified to single check
if schema and 'properties' in schema:
...
social_protection/custom_filters.py
Outdated
| for key in properties: | ||
| property = properties[key] |
There was a problem hiding this comment.
you can iterate on dict.items() instead of taking keys and then loading values
|
@dborowiecki Please re-check PR |
TICKET: https://openimis.atlassian.net/browse/CM-69
related PR: openimis/openimis-be-core_py#185
PR provides the implementation of wizard interface for BenefitPlan instances.
NOTE: Tests are failing due to lack of merging such PR from core module openimis/openimis-be-core_py#181