-
Notifications
You must be signed in to change notification settings - Fork 105
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
Feat: merge ODK central work #6337
Feat: merge ODK central work #6337
Conversation
beee509
to
6aca5b0
Compare
@jmcameron I requested a review too early. There's some code I'm going to try and migrate out of this PR into the ODK Central API library. I'll ping you when I'm ready for a review. |
41e32f2
to
484589d
Compare
@jmcameron this is ready for review. |
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 not sync with the server. Tried logging manually and that worked. But when I carefully enter the ODK central server credentials and try "Sync Enterprise", I get 403 errors.
484589d
to
7b51ec6
Compare
@jmcameron thanks for the review. I think I gave you the wrong credentials. I have sent you some new ones! |
This commit implements the ODK Central link with BHIMA, developped during the `prosani-sprint`. The goal is to have a functional, optional link to ODK Central that can be merged into the BHIMA repository. Closes Third-Culture-Software#6235.
This commit skips users in the database without email addresses. Closes Third-Culture-Software#6322.
This commit disables the sync buttons when synchronisation is not finished yet.
This commit adds better loading and default text to the ODK Central configuration page.
This commit cleans up the ODK Central work, having moved some items out of the odk-central file to be put in the ODK Central API library.
This commit cleans up old code and gets the ODK portion totally ready for submission.
5207504
to
3fcbeb2
Compare
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 found the term "ODK Central" a little vague. As I was trying to figure out ODK, it was not obvious if this was part of BHIMA or not. So I suggest renaming this to "BHIMA ODK Server" or something similar.
Obviously this is a bare-bones ODK server. However (in another PR), we need a registry for ODK forms, submissions, aggregated data, etc.
I did a basic review of the code. See the inline code comments.
I also tested this (per the procedure outlined in the PR description) with assistance from @jniles. Other than an online server database issue that @jniles fixed behind the scenes, it all worked nicely.
We found that the QR code for the ODK server settings works well. However, the BHIMA lot barcodes seem to be a little small for reliable scanning on my cell phone. So I suggest increasing the lot barcode display. If that is not done in this PR, I suggest creating a new issue for it.
In practice, I suspect the layout of this page will evolve. It seems like it should perhaps be a "ODK Campaign" registry and the Settings and sync actions might fall under the main Menu button?
server/models/bhima.sql
Outdated
@@ -178,7 +178,8 @@ INSERT INTO unit VALUES | |||
(302, 'Cost Centers Accounts Report','TREE.COST_CENTER_ACCOUNTS_REPORT','Report of cc accounts values', 286,'/reports/cost_center_accounts'), | |||
(303, 'Cost Centers Balance Report','TREE.COST_CENTER_INCOME_EXPENSE_REPORT','Report of cc balance', 286,'/reports/cost_center_income_and_expense'), | |||
(304, '[SETTINGS] Settings', 'TREE.PAYROLL_SETTINGS', 'Payroll Settings', 57, '/payroll/setting'), | |||
(305, 'Avg Medical Costs Per Patient', 'TREE.AVERAGE_MED_COST_REPORT', 'Report of avg med costs', 282, '/reports/avg_med_costs_per_patient'); | |||
(305, 'Avg Medical Costs Per Patient', 'TREE.AVERAGE_MED_COST_REPORT', 'Report of avg med costs', 282, '/reports/avg_med_costs_per_patient'), | |||
(306, '[SETTINGS] ODK Settings', 'TREE.ODK_SETTINGS', 'ODK Settings', 1, '/admin/odk-settings'); |
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.
While using the [Settings] option to add the gear icon for the ODK Settings looks nice, it a bit odd in the context of the other Administration menu items. Several of the items in that menu are for settings (eg Enterprise) and are not marked with the gear icon. So the ODK Settings is not consistent with the rest. So I suggest either removing [SETTINGS] or adding to other items in the Administration menu that are for settings. Also, in reality, the ODK Settings item is about more than settings. Only the left most panel is for settings. The rest are for actions, so this page is not ALL settings.
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.
Yeah, this is a good point. Let me do that.
* @description: update migrate.sql file for | ||
* @date: 2022-01-24 | ||
*/ | ||
UPDATE report SET `report_key` = 'analysis_auxiliary_cashbox', `title_key` = 'REPORT.ANALYSIS_AUX_CASHBOX.TITLE' |
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.
It was not obvious why these got deleted.
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.
Probably a failed merge. Let me fix it.
@jmcameron thanks for the review. ODK Central is the official name: https://docs.getodk.org/central-setup/. I do agree that it is confusing. They used to have a different server called ODK Aggregate, which also was a pretty non-googleable name. |
This commit refactors code from the review.
Got it. Somehow I thought "ODK Central" was a server that we actually set up ourselves. |
This commit automatically assigns teh forms to the users on creation of the form.
@jmcameron got the assignment bug fixed. If you want to test it again, feel free. Otherwise, I think this is ready to go. |
Bors r+ |
Canceled. |
@jniles It was not obvious why the merge attempt failed. I'll try again. |
bors r+ |
Build succeeded: |
Thanks! |
This PR adds in ODK Central functionality to the BHIMA server. Here is what the ODK administration interface looks like:
You'll need to set your own username/password and ODK Central URL to test it out.
This installation is very bare-bones. The way this works is somewhat documented in #6235 (comment).
This PR is plucked from work done by @jniles and @mbayopanda in
sprint-prosani
.Closes #6235.
Closes #6302.
Closes #6322.
TESTING (based on session with @jniles )