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

O3-614: Added support for AMPATH Forms Translations. #191

Merged
merged 19 commits into from
Dec 21, 2022
Merged

O3-614: Added support for AMPATH Forms Translations. #191

merged 19 commits into from
Dec 21, 2022

Conversation

icrc-jofrancisco
Copy link
Contributor

@icrc-jofrancisco icrc-jofrancisco commented Jul 22, 2022 •
edited by mks-d
Loading

Support for loading AMPATH translation files using the initializer.
The file will be saved as ClobDatatypeStorage in order to support multiple forms.

@icrc-jofrancisco icrc-jofrancisco marked this pull request as ready for review July 25, 2022 14:31
@rbuisson
Copy link
Member

rbuisson commented Sep 6, 2022 •
edited
Loading

@icrc-jofrancisco , I guess the domain would be better named "Ampath Forms Translations"

Addtionally, could you make sure to provide the update in the readme?

@gracepotma
Copy link
Contributor

Perhaps @ibacher could review this?

@icrc-jofrancisco icrc-jofrancisco mentioned this pull request Nov 9, 2022
Closed
1 task
mks-d
mks-d requested changes Nov 21, 2022
View reviewed changes
Copy link
Member

@mks-d mks-d left a comment

Choose a reason for hiding this comment

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

Thanks @icrc-jofrancisco, however we need a couple of things:

  1. Unit tests.
  2. Amendments to the README, including the line for the upcoming release notes, and this should probably be for 2.5.0 not 2.4.1, @rbuisson?
  3. Possible addition of an ad-hoc README for this new domain, others to weigh in as to this is overkill or not.

@ibacher
Copy link
Member

ibacher commented Nov 21, 2022

Possible addition of an ad-hoc README for this new domain, others to weigh in as to this is overkill or not.

I actually think documenting this domain is crucial.

@icrc-jofrancisco
Copy link
Contributor Author

Hi @ibacher @rbuisson @mks-d,
I added some tests, changed some names and completed the readme files.
I'm open to new suggestions or changes that might be needed.

Thanks,

@mks-d mks-d changed the title support-ampath-translations: Ampath Translations Loader added. O3-1325: Added support for AMPATH Forms Translations. Nov 26, 2022
@mks-d mks-d changed the title O3-1325: Added support for AMPATH Forms Translations. O3-614: Added support for AMPATH Forms Translations. Nov 29, 2022
mks-d
mks-d reviewed Nov 29, 2022
View reviewed changes
Copy link
Member

@mks-d mks-d left a comment

Choose a reason for hiding this comment

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

@icrc-jofrancisco did you see that the GitHub Actions build is failing? Could you have a look?

@rbuisson
Copy link
Member

@ibacher , didn't you mention that the AMPATH forms translation files should be attached to forms via a form resource?

rbuisson
rbuisson reviewed Dec 14, 2022
View reviewed changes
readme/ampathformstranslations.md Outdated
Comment on lines 29 to 32
"translations": {
"en": ["global-uuid-resource-en", "test-form-en"],
"fr": ["global-fr-uuid-resource-fr", "test-form-fr"]
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need that even when using the form resource?
I was under the impression that saving the translations as form resources would avoid having to specify the said translation file UUID in the form.

Copy link
Member

@Ruhanga Ruhanga Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Correct @rbuisson. Specifying this form translation uuid is not a requirement. To make this non umbigious, let me remove this section.

ibacher
ibacher reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Thanks @Ruhanga! Functionality-wise, this looks great, just a few comments on polishing this up.

api/src/main/java/org/openmrs/module/initializer/api/loaders/AmpathFormsTranslationsLoader.java Outdated Show resolved Hide resolved
api/src/main/java/org/openmrs/module/initializer/api/loaders/AmpathFormsTranslationsLoader.java Outdated Show resolved Hide resolved
api/src/main/java/org/openmrs/module/initializer/api/loaders/AmpathFormsTranslationsLoader.java Outdated Show resolved Hide resolved
...va/org/openmrs/module/initializer/api/form/AmpathFormsTranslationsLoaderIntegrationTest.java Outdated Show resolved Hide resolved
...va/org/openmrs/module/initializer/api/form/AmpathFormsTranslationsLoaderIntegrationTest.java Outdated Show resolved Hide resolved
...va/org/openmrs/module/initializer/api/form/AmpathFormsTranslationsLoaderIntegrationTest.java Outdated Show resolved Hide resolved
...va/org/openmrs/module/initializer/api/form/AmpathFormsTranslationsLoaderIntegrationTest.java Show resolved Hide resolved
...esources/testAppDataDir/configuration/ampathformstranslations/test_form_translations_fr.json Show resolved Hide resolved
readme/ampathforms.md Outdated Show resolved Hide resolved
readme/ampathformstranslations.md Outdated Show resolved Hide resolved
@ibacher
Copy link
Member

ibacher commented Dec 16, 2022

Code looks good... Not sure about the build failure...

Ruhanga
Ruhanga reviewed Dec 21, 2022
View reviewed changes
Copy link
Member

@Ruhanga Ruhanga left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The build fails randomly with github actions but passes locally @ibacher.

@rbuisson
Copy link
Member

rbuisson commented Dec 21, 2022 •
edited
Loading

@Ruhanga , are you able to figure why the GitHub Actions build is failing?

@rbuisson
Copy link
Member

GitHub Actions seems to be failing to get the org.bahmni.module:bahmnicore-api:jar:0.93-1.1.0 dependency. If removing this dependency from your local Maven repo, you should face the same issue.

@rbuisson
Copy link
Member

rbuisson commented Dec 21, 2022 •
edited
Loading

Adding this to pom.xml <repositories> section shall fix it.

        <repository>
            <id>mks-repo</id>
            <name>Mekom Solutions Nexus</name>
            <url>https://nexus.mekomsolutions.net/repository/maven-public</url>
        </repository>

@rbuisson rbuisson merged commit f5966a6 into mekomsolutions:master Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ruhanga Ruhanga Ruhanga left review comments

@mks-d mks-d mks-d requested changes

@rbuisson rbuisson rbuisson left review comments

@ibacher ibacher ibacher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@icrc-jofrancisco @rbuisson @gracepotma @ibacher @mks-d @Ruhanga