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.

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

⬆️ Move the FHIR commons utils dependency to the plugin and re-enable the spotless plugin #13

Open
wants to merge 1 commit into
base: permission-checker-and-data-access-checker
Choose a base branch
from

Conversation

dubdabasoduba
Copy link
Member

Description of what I changed

E2E test

TESTED:

Please replace this with a description of how you tested your PR beyond the
automated e2e/unit tests.

Checklist: I completed these to help reviewers :)

  • I have read and will follow the
    review process.

  • I am familiar with Google Style Guides for the language I have coded in.

    No? Please take some time and review
    Java and
    Python style guides. Note,
    when in conflict, OpenMRS style guide overrules.

  • My IDE is configured to follow the Google
    code styles.

    No? Unsure? ->
    configure your IDE.

  • I have added tests to cover my changes. (If you refactored existing
    code that was well tested you do not have to add tests)

  • I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

  • All new and existing tests passed.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@@ -149,12 +149,6 @@
<version>1.18.24</version>
</dependency>

<dependency>

Choose a reason for hiding this comment

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

Have you added this in the plugins module?
@dubdabasoduba

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was already available. So I don't think the server needed it.

Choose a reason for hiding this comment

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

Right, that makes sense.
Let me test the PR first and then we can merge.

Choose a reason for hiding this comment

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

I have tested the PR, we have to include this as a dependency in the server module because it fails with ClassNotFoundException.
@dubdabasoduba
image

Copy link
Member Author

Choose a reason for hiding this comment

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

This is interesting, we want to abstract all the OpenSRP-related work from the server. We do not want to update the server just for the OpenSRP use case. I think we need to review how we have written to code to decouple this requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants