Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding XML Version #7319

Merged
merged 11 commits into from
Apr 13, 2022
Merged

Adding XML Version #7319

merged 11 commits into from
Apr 13, 2022

Conversation

aodhan-domhnaill
Copy link
Contributor

@aodhan-domhnaill aodhan-domhnaill commented Sep 17, 2021 •
edited
Loading

Description

medic/cht-conf#395

#7561

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@aodhan-domhnaill aodhan-domhnaill mentioned this pull request Sep 17, 2021
Closed
garethbowen
garethbowen reviewed Sep 19, 2021
View reviewed changes
webapp/src/ts/services/enketo.service.ts Outdated
@@ -570,6 +571,7 @@ export class EnketoService {
.map((idx, element) => {
const docToStore:any = this.enketoTranslationService.reportRecordToJs(getOuterHTML(element));
docToStore._id = getId(Xpath.getElementXPath(element));
docToStore.xmlVersion = xmlVersion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a draft, but I thought I'd throw a comment in...

This is setting the xmlVersion in the child docs being created by the report but not in the report itself. This is potentially useful but we don't store anything about how the child doc was created, not even the form ID, so it's potentially not useful on its own. The primary place this should be set is in the create function where the report itself is created, right beside where we set the form property.

For CouchDB properties we use snake case for naming (like reported_date below). This applies to both the form and report docs. There are some couterexamples you can find but we're standardising at least on new properties.

Finally I'm not sure xmlVersion will make much sense to someone looking at the report who doesn't know the context. I think form_version would be better and be obvious when stored beside form (the form's ID).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. So I understand, the change I made in medic/cht-conf#431 will cause the get(internalId) function here to return a doc Promise that contains the new field xmlVersion that I add in medic/cht-conf#431

Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should do! You'll obviously need to get the CHT running in a developer environment to test this end to end.

@aodhan-domhnaill

This comment was marked as resolved.

Sign in to view
@aodhan-domhnaill

This comment was marked as resolved.

Sign in to view
@aodhan-domhnaill aodhan-domhnaill mentioned this pull request Nov 8, 2021
Merged
@garethbowen

This comment was marked as resolved.

Sign in to view
@aodhan-domhnaill aodhan-domhnaill marked this pull request as ready for review March 29, 2022 13:48
aodhan-domhnaill and others added 9 commits April 4, 2022 14:48
@aodhan-domhnaill
Simplifying changes
660422a 
Fix tests
@m5r
Re-set the enketo form's language when a DOM node is added to the form (
1e87f98 
#7520)
@m5r
Fix eslint error in tests/e2e/api/controllers/users.js (#7541)
a3d1bd0
@latin-panda
7529 - Increase Enketo's upload file size limit and add validation wh…
c6680c5 
…en submitting form (#7532)

Ticket: #7529

This commit:
- Increases the Enketo's upload file size limit to 30MB, leaving a couple of megas for other things.
- Adds a validation that sums all form's attachments and ensures it's not more than 30MB.
- Shows error message to the user when limit is exceeded by using the snackbar.
@mrjones-plip
Minor Dependency updates for 3.15 (redux) (#7539)
71f54cf 
* update share-lib/bulk-docs-utils

* update share-lib/calendar-interval

* update share-lib/cht-script-api

* update share-lib/contact-types-utils

* update share-lib/infodoc

* update share-lib/lineage 1 of 2

* update share-lib/lineage 2 of 2

* update share-lib/message-utils

* update share-lib/outbound

* update share-lib/phone-number

* update share-lib/purging-utils

* update share-lib/registration-utils

* update share-lib/rules-engine

* update share-lib/search

* update share-lib/server-checks

* update share-lib/settings

* update share-lib/task-utils

* update share-lib/tombstone-utils

* update share-lib/transitions

* update share-lib/translation-utils

* update share-lib/validation

* update share-lib/view-map-utils

* fix lodash mismatch in share-lib/rules-engine

* update admin dependencies

* revert lodash in admin

* dependencies for api 1 of 2

* update sentinel dependencies

* update webapp minor dependencies

* revert @medic file:.. dependency in message-utils per feedback

* un-revert @medic file:.. dependency in message-utils b/c build broke

* update version in package and lock to 3.15.0

* right proper  revert @medic file:.. dependency in message-utils per feedback
@jkuester
Fix labeling Issues with Default and Standard Config (#7479)
826f18b
@jkuester
Fix node version in DEVELOPMENT.md (#7554)
3e66d2d
@jkuester
Fix docker run command for CouchDB in DEVELOPMENT.md (#7549)
d0c58da
Fixing a merge error
180fe76 
Fixing unit test

Fixing unit tests

Fix Linting

Fixing unit tests
@aodhan-domhnaill aodhan-domhnaill force-pushed the 395-xml-form-version branch 5 times, most recently from e2710d7 to ac8137f Compare April 5, 2022 18:50
@m5r m5r self-requested a review April 12, 2022 08:30
@aodhan-domhnaill aodhan-domhnaill removed the request for review from m5r April 12, 2022 12:52
@aodhan-domhnaill aodhan-domhnaill merged commit fa40f74 into master Apr 13, 2022
@aodhan-domhnaill aodhan-domhnaill deleted the 395-xml-form-version branch April 13, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garethbowen garethbowen garethbowen left review comments

@m5r m5r m5r approved these changes

Assignees

@m5r m5r

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aodhan-domhnaill @garethbowen @m5r @latin-panda @mrjones-plip @jkuester