Added BulkEditChange.edited_value #35806
Conversation
orangejenny
requested review from
millerdev,
biyeun,
AmitPhulera and
gherceg
|
|
||
| def edited_value(self, case): | ||
| simple_transformations = { | ||
| EditActionType.REPLACE: lambda x: self.replace_string, |
There was a problem hiding this comment.
self.replace_all_string is what should be used here
There was a problem hiding this comment.
What's the purpose of replace_string?
There was a problem hiding this comment.
self.replace_string is used in find and replace. I can combine the two if you feel bothered by it, but the two will be separate fields in the form
There was a problem hiding this comment.
Yes, I think they should be combined.
| return old_value.replace(self.find_string, self.replace_string) | ||
|
|
||
| if self.action_type == EditActionType.RESET: | ||
| raise UnsupportedActionException(f"{EditActionType.RESET} is not applied by calling edited_value") |
There was a problem hiding this comment.
this should just return the value of the current case property value. I think this should be implemented.
There was a problem hiding this comment.
The intention is to reset all previous edits? Based on the spec, I thought it only reset the previous action.
There was a problem hiding this comment.
no reset all edits, not the previous action. It's the equivalent of hitting the "x" in the column or "Undo all edits" in the menu for the action
There was a problem hiding this comment.
Then why would it go back to the original value of the case property, when it may have had additional edits?
I'm not addressing reset right now.
| def edited_value(self, case): | ||
| simple_transformations = { | ||
| EditActionType.REPLACE: lambda x: self.replace_string, | ||
| EditActionType.FIND_REPLACE: lambda x: x.replace(self.find_string, self.replace_string), |
There was a problem hiding this comment.
does this support regular expressions?
this was what I had to do to support a regex find and replace in the prototype
commcare-hq/corehq/apps/prototype/forms/data_cleaning.py
Lines 334 to 338 in 0167849
There was a problem hiding this comment.
No. Didn't we agree regexes were out of scope for v1? https://docs.google.com/document/d/1mboGuJDQmC3zREoGtWmUHRrZyDX7S1JlEZ7HSuLowfc/edit?disco=AAABawlbueE
There was a problem hiding this comment.
self.use_regex will note whether the self.find_string is a regular expression
There was a problem hiding this comment.
@orangejenny regexes were out of scope for filters, but not find/replace
There was a problem hiding this comment.
I'll add it in a future PR. The purpose of this PR is to get enough functionality that backend tests can run.
corehq/apps/data_cleaning/models.py
Outdated
| if self.action_type == EditActionType.COPY_REPLACE: | ||
| old_value = case.get_case_property(self.copy_from_property) | ||
| return old_value.replace(self.find_string, self.replace_string) |
There was a problem hiding this comment.
this is not the intention of COPY_REPLACE. This should return the value of case.get_case_property(self.copy_from_property) WITHOUT any find and replace action. It's just replacing the value of the current property with the value from the copy_from_property
| def test_find_replace(self): | ||
| change = BulkEditChange( | ||
| property='favorite_beach', | ||
| action_type=EditActionType.FIND_REPLACE, | ||
| find_string='Playa', | ||
| replace_string='Punta', | ||
| ) | ||
| self.assertEqual(change.edited_value(self.case), 'Punta Bastimento') |
There was a problem hiding this comment.
should also add a test for regular expressions
There was a problem hiding this comment.
(a different test where use_regex=True)
There was a problem hiding this comment.
Not implementing regular expressions in this PR.
| 'nearest_ocean': 'atlantic', | ||
| 'town': 'Isabel Segunda', | ||
| 'favorite_beach': 'Playa Bastimento', | ||
| 'art': ' :) ', |
There was a problem hiding this comment.
there are many types of whitespaces. we should be sure to include \t and \n, as those are often the problematic ones (esp \n)
| change = BulkEditChange( | ||
| property='town', | ||
| action_type=EditActionType.REPLACE, | ||
| replace_string='Esperanza', |
There was a problem hiding this comment.
| replace_string='Esperanza', | |
| replace_all_string='Esperanza', |
There was a problem hiding this comment.
Ignoring since these fields are being combined.
| find_string='Bastimento', | ||
| replace_string='Yallis', |
There was a problem hiding this comment.
not the intention. these values will never be present. the result should be the value of favorite_beach
There was a problem hiding this comment.
Resolved in b5dfc1f
| def test_title_case(self): | ||
| change = BulkEditChange( | ||
| property='nearest_ocean', | ||
| action_type=EditActionType.TITLE_CASE, | ||
| ) | ||
| self.assertEqual(change.edited_value(self.case), 'Atlantic') |
There was a problem hiding this comment.
what about strings with multiple words and whitespace characters in between?
There was a problem hiding this comment.
It's directly calling a built-in python method. I'm not concerned about exhaustively testing that.
| @@ -0,0 +1,103 @@ | |||
| from django.test import TestCase | |||
There was a problem hiding this comment.
could this file be renamed to test_changes.py?
There was a problem hiding this comment.
Why? It's testing model behavior.
There was a problem hiding this comment.
i'm going to have other model tests in different files
There was a problem hiding this comment.
Definitely feel free to rename as the code evolves.
| @@ -257,3 +259,28 @@ class BulkEditChange(models.Model): | |||
|
|
|||
| class Meta: | |||
| ordering = ["created_on"] | |||
|
|
|||
| def edited_value(self, case): | |||
There was a problem hiding this comment.
nit: I always prefer a verb for methods, especially if they return something. maybe get_edited_value?
There was a problem hiding this comment.
I'm not a fan of prefixing methods with get_.
There was a problem hiding this comment.
i think it's clearer because otherwise, someone could misread it as edit_value rather than retrieving the edited value
Technical Summary
Cherry-pick from the data cleaning backend branch.
Adds a BulkEditChange.edited_value to do the basic string transformations.
This does not handle the RESET action, that'll come later.
Safety Assurance
Safety story
Adds new code and tests, but nothing calls the new code.
Automated test coverage
In PR.
QA Plan
No.
Rollback instructions
Labels & Review