Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Support for Case Deletion #33831
base: master
Are you sure you want to change the base?
Add Support for Case Deletion #33831
Changes from 8 commits
3570ec2
580b5a1
556d299
5f1c4c4
707da7d
8847642
52b9443
870f8ca
f78631c
d96c8a9
4e36269
3481e4f
588b377
e91c974
53852ac
f40be4b
3c00173
fe480b3
0f5e737
2ee8572
52c42bb
98dd353
86f5deb
18fd6ba
9c65a10
83872d4
76c6760
e5b25f8
09ad4ff
b93a8cb
cf79b54
d3197eb
53f3877
1e49ef8
e1fc391
e5a2f9b
9538140
55d63a3
37d3f06
23f27df
2565d07
3a9d827
04bdb7b
1b1b86d
1b4b297
c9b879e
b062be8
bfa16b7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
perhaps a dataclass or similar data structure could be used to give this better structure.
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 not really familiar with using dataclasses but can they be unwrapped like a dict in an html template? The sole use of this dictionary is to display the case/form information in the exact order it's stored in so I'm not sure if converting it to a dataclass would really help.
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.
@minhaminha I think something like a
namedtuple
would be helpful, here is an example:commcare-hq/corehq/apps/cleanup/management/commands/undelete_docs.py
Line 13 in 72e347c
you can then call
. _asdict()
on yournamedtuple
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.
dataclass and attrs objects (a richer form of the same) can both be converted to dicts, although it should not be necessary. Just pass the object(s) out to the template and reference properties normally.
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.
@millerdev I always found
namedtuple
nice due to the shorter notation for a quick dataclass-esq object for simple use cases like this. curious where your preference for an attrs object comes in vs anamedtuple
? Usually I would use the former if I wanted optional fields and methods, butnamedtuple
for very quick named attributes that were always requiredThere 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 find dataclasses and attrs to support more natural class syntax than
namedtuple
. There is tons more flexibility such as field defaults likefactory=list
, it's trivial to add methods, etc. Also, something is sticking in my mind thatnamedtuple
has a performance overhead penalty, but I could be wrong about that. SO also points out thatThere 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.
One other thing is namedtuple doesn't support default values which is often useful.
Also, I find most IDEs quite bad at backlinking to namedtuple definitions since they are just a variable and have no static class definition.
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.
Could this be a boolean rather than raw HTML?
Similar to my comment above - perhaps a data structure would be better than a string.
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.
second this! raw HTML in python should always be avoided
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.
to tie in mentions of a data structure. perhaps something like
and then in the template, you can call on the properties by iterating through a list of structured items rather than
key, value
pairs fromaffected_cases.items
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.
Except use
attrs
rather thannamedtuple
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.
How long does this take to run? Wondering it would make sense to offload it to celery
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.
From my testing, It doesn’t take long at all. There’s also a limit on how many cases it can look at/return so the lag shouldn’t really be noticeable.