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

TRUNK-5671 Replace Provider hbm mapping file with annotations #3031

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

wluyima
Copy link
Member

@wluyima wluyima commented Oct 18, 2019

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-5671

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit
    (the number above, next to the 'Commits' tab is 1).

    No? -> read here on how to squash multiple commits into one

  • My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

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

    No? -> write tests and add them to this commit git add . && git commit --amend

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

    No? -> execute above command

  • All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

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

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

@dkayiwa
Copy link
Member

dkayiwa commented Oct 20, 2019

@wluyima did you take a look at the travis build failure?

@coveralls
Copy link

coveralls commented Oct 22, 2019

Coverage Status

Coverage decreased (-0.01%) to 59.857% when pulling 3a4cd8f on wluyima:TRUNK-5671 into 0aacf19 on openmrs:master.

@Table(name = "provider")
@SecondaryTable(name = "provider_unused_fields")
@AttributeOverrides({ @AttributeOverride(name = "name", column = @Column),
@AttributeOverride(name = "description", column = @Column(table = "provider_unused_fields", name = "description", insertable = false, updatable = false)) })
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 wanna add a comment explaining why we introduced provider_unused_fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

What would be the effect of using the same unused_fields table like we already have users_unused_fields instead of creating a new one for each of these scenarios?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be able to work

@dkayiwa
Copy link
Member

dkayiwa commented Oct 22, 2019

When i run these changes with the providermanagement module, i get this error: Caused by: org.hibernate.MappingException: No discriminator found for org.openmrs.module.providermanagement.Provider. Discriminator is needed when 'single-table-per-hierarchy' is used and a class has subclasses

@dkayiwa
Copy link
Member

dkayiwa commented Oct 27, 2019

Did you see the merge conflicts?

@wluyima
Copy link
Member Author

wluyima commented Nov 6, 2019

Fixed

@dkayiwa
Copy link
Member

dkayiwa commented Nov 6, 2019

@wluyima i have just run these changes again on the reference application and i still get this error:

Caused by: org.hibernate.MappingException: No discriminator found for org.openmrs.module.providermanagement.Provider. Discriminator is needed when 'single-table-per-hierarchy' is used and a class has subclasses

https://pastebin.com/sM9CDdhG

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations
@wluyima
Copy link
Member Author

wluyima commented Nov 8, 2019

Should be fine now

@dkayiwa
Copy link
Member

dkayiwa commented Nov 8, 2019

When i run these changes with the reference application, on selecting a patient in order to go to their dashboard, i get this error: https://pastebin.com/t5RVEZV5

@wluyima
Copy link
Member Author

wluyima commented Nov 9, 2019

Well, if it too many joins, then it's bound to happen with or without this change

@dkayiwa
Copy link
Member

dkayiwa commented Nov 11, 2019

Now that this change exposes it, what would you suggest as the way forward?

@dkayiwa dkayiwa merged commit 3a66cab into openmrs:master Nov 20, 2019
wluyima added a commit to wluyima/openmrs-core that referenced this pull request Nov 21, 2019
…s#3031)

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations
wluyima added a commit that referenced this pull request Nov 21, 2019
TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations
prathamesh-mutkure pushed a commit to prathamesh-mutkure/openmrs-core that referenced this pull request Dec 14, 2019
…s#3031)

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations

TRUNK-5671 Replace Provider hbm mapping file with annotations
wluyima added a commit to wluyima/openmrs-core that referenced this pull request Dec 19, 2019
wluyima added a commit to wluyima/openmrs-core that referenced this pull request Dec 19, 2019
prathamesh-mutkure pushed a commit to prathamesh-mutkure/openmrs-core that referenced this pull request Dec 25, 2019
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.

3 participants