-
Notifications
You must be signed in to change notification settings - Fork 133
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 entities #2504
Add support for entities #2504
Conversation
2be3074
to
5230e72
Compare
5230e72
to
cc38c80
Compare
15a525f
to
f688576
Compare
211f412
to
23bea2b
Compare
090ab0c
to
12dbb9d
Compare
fb3a381
to
82b207e
Compare
5e67dbc
to
c755de8
Compare
d53d332
to
7989a3f
Compare
aec409e
to
bb353fc
Compare
0841c55
to
908be16
Compare
184a93a
to
25bfc41
Compare
version="2022110901", | ||
xml=data_dict.xml, | ||
json=json.dumps(data_dict.json), | ||
) |
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 we have used onadata.libs.utils.logger_tools.create_xform_version(xform: Xform, user: User)
?
Perhaps, could we consider adding publish_md_form()
similar to the publish_xls_form()
and publish_xml_form()
functions under onadata.libs.utils.logger_tools
that have the same signature and all create the XFormVersion
object of the form they publish? This will mean we update self._publish_markdown()
to use the new publish_md/markdown_form()
function and an XFormVersion
object will always be created when publishing the form.
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 we have used onadata.libs.utils.logger_tools.create_xform_version(xform: Xform, user: User)?
@ukanga Since this is a test we should assume onadata.libs.utils.logger_tools.create_xform_version
does not exist in order to avoid coupling the tests with actual implementations. Also, using TDD, we wouldn't expect onadata.libs.utils.logger_tools.create_xform_version
to exist
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.
@kelvin-muchiri There are several places where the XFormVersion
is directly created. So, if we apply this change only in self._publish_markdown()
, the repetition can 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.
I agree we may not need to create publish_md/markdown_form()
unless we see future potential for it. The main reason to consider it is to ensure we do not repeat or forget some steps, like, in this case, the function self._publish_markdown()
forgot to create this object that should always be present when publishing a form.
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.
@ukanga I resolved this by removing the redundant _publish_markdown
in TestAbstractViewSet
and inheriting TestBase
onadata/apps/logger/migrations/0014_rename_entity_created_at.py
Outdated
Show resolved
Hide resolved
cf9b2fb
to
73b8206
Compare
e532ffc
to
8114a5c
Compare
This resolves the error django.db.migrations.exceptions.IrreversibleError when unapplying the migration
c8514e2
to
d8b3442
Compare
Changes / Features implemented
EntityList
,RegistrationForm
,FollowUpForm
modelsxml.dom.minidom.parseString
withdefusedxml.minidom.parseString
to avoid threats of XML attacks https://pypi.org/project/defusedxmlGenericExport
for storing export related data for EntityList datasets and any other objects in future that may need to have exportsSteps taken to verify this change does what is intended
Side effects of implementing this change
Before submitting this PR for review, please make sure you have:
Closes #2500