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-5338 Add changeset to create or modify conditions table #2662

Merged
merged 1 commit into from Jun 14, 2018

Conversation

paradisekelechi
Copy link
Contributor

Description of what I changed

Added liquibase changeset to add a new conditions table or modify an existing one.

Issue I worked on

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

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

@paradisekelechi paradisekelechi force-pushed the TRUNK-5338 branch 2 times, most recently from d9394ce to 8fc2e69 Compare May 1, 2018 21:24
@dkayiwa
Copy link
Member

dkayiwa commented May 2, 2018

@paradisekelechi did you actually test this out by running an instance of openmrs, go through the database setup wizard, complete it, check the database to confirm all is changed as expected?

@paradisekelechi
Copy link
Contributor Author

paradisekelechi commented May 2, 2018

@dkayiwa
I actually tested it. I ran it and the changes were effected accordingly on the already existing conditions table. After that, I dropped the table and ran and checked the other changeset.

However, I do not know about the database setup wizard. Could you share resources with respect to that?

@dkayiwa
Copy link
Member

dkayiwa commented May 2, 2018

In the liquibasechangelog table, delete the row with this changeset id. Then restart openmrs and tell me what you get in the browser.

@paradisekelechi
Copy link
Contributor Author

About that, I did that a lot of times until I no longer have errors on the console.
I'd run that again and update you accordingly.

@dkayiwa
Copy link
Member

dkayiwa commented May 2, 2018

How are you dealing with the data migration? Didn't some columns store data in a different format than that of the old table?

@paradisekelechi
Copy link
Contributor Author

The only tables that changed are previous_condition_id, status and concept_id and they only changed in name and not datatype.

@paradisekelechi
Copy link
Contributor Author

paradisekelechi commented May 2, 2018

What I am presently concerned about is adding foreign key constraints after converting an old conditions table to the new one. . .

@dkayiwa
Copy link
Member

dkayiwa commented May 2, 2018

  1. Can you revert to the old table.
  2. Create around 5 conditions from the user interface when running on openmrs-core versions 2.1.3 or lower.
  3. Now switch to openmrs-core 2.2.0-SNAPSHOT to have your changesets run
  4. Check to see if the data is migrated and expected and also displayed in the UI using the latest snapshot versions of the ermapi and coreapps modules.

<comment>
Modify conditions table to match condition object in the core platform if the conditions table already exists in the database
</comment>
<renameColumn columnDataType="int" newColumnName="previous_version" oldColumnName="previous_condition_id" tableName="conditions"/>
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to have these as separate changesets with their respective comments being explicit about what they do e.g Rename tableA.x column to y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Done

<renameColumn columnDataType="varchar(255)" newColumnName="clinical_status" oldColumnName="status" tableName="conditions"/>
<renameColumn columnDataType="int" newColumnName="condition_coded" oldColumnName="concept_id" tableName="conditions"/>

<addColumn tableName="conditions">
Copy link
Member

Choose a reason for hiding this comment

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

This should also be a separate changeset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Noted

<addColumn tableName="conditions">
<column name="condition_coded_name" type="int"/>
</addColumn>
</changeSet>
<changeSet id="20180214-1000" author="Kelechi+iheanyi">
Copy link
Member

Choose a reason for hiding this comment

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

It's good to change the id when you change the changeset contents so that this doesn't break other developer's working copies or our CI servers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Done

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the change

</preConditions>
<comment>
Add condition table
Add conditions table if the table does not exist in the database
Copy link
Member

Choose a reason for hiding this comment

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

Add conditions table should be enough, you don't have to include the precondition in the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Noted

@paradisekelechi paradisekelechi force-pushed the TRUNK-5338 branch 2 times, most recently from 63dd931 to 111a5af Compare May 3, 2018 14:07
@dkayiwa
Copy link
Member

dkayiwa commented May 4, 2018

@paradisekelechi have you tried to run a fresh setup of openmrs with these changes?

@dkayiwa
Copy link
Member

dkayiwa commented May 4, 2018

@paradisekelechi if you do, you should get this error: https://pastebin.com/QxxXu5h6

@paradisekelechi
Copy link
Contributor Author

@dkayiwa
I have run a fresh setup of the core platform following the steps you highlighted. I did not get that error. The changesets ran successfully without errors

@dkayiwa
Copy link
Member

dkayiwa commented May 4, 2018

Do you have the reference application modules loaded?

@paradisekelechi
Copy link
Contributor Author

Yes I do have the reference application modules loaded

@dkayiwa
Copy link
Member

dkayiwa commented May 4, 2018

That does not look like a fresh setup. For the emrapi module would attempt to create a conditions table which should fail because the core platform already created it.

@paradisekelechi
Copy link
Contributor Author

@dkayiwa
I took the following steps with the following results:

  1. After switching to versions 2.1.3, I setup a new sdk instance and ran it which created a new db. It had the conditions table.
  2. I went ahead to add 5 new conditions successfully to the db.
  3. I switched back to openmrs-core 2.2.0-SNAPSHOT and ran the server using jetty.
  4. The new changesets ran which updated the conditions table and marked ran the changeset to add a new conditions table.
  5. Started the app and checked the conditions I had created. They were still showing. However, on that view, I get this error on the console : https://pastebin.com/MwK4WZqj

@dkayiwa
Copy link
Member

dkayiwa commented May 4, 2018

  1. Delete your database
  2. Delete your runtime properties file.
  3. Now run openmrs-core 2.2.0-SNAPSHOT

@paradisekelechi
Copy link
Contributor Author

@dkayiwa
I have done the above. This is the output I get in the console for these changesets: https://pastebin.com/Fr0w4nt9 .
On checking the db created, the conditions table is there with the changes effected accordingly.

@dkayiwa
Copy link
Member

dkayiwa commented May 4, 2018

What exactly did you do? Can you list the steps here?

@paradisekelechi
Copy link
Contributor Author

  1. I dropped the previous databases I had.
  2. I deleted the openmrs-runtime.properties file
  3. I started the server and followed through on the steps which created a new db and updated it by running the changesets

@dkayiwa
Copy link
Member

dkayiwa commented May 4, 2018

As you did that, which version of openmrs-core were you running?

@paradisekelechi
Copy link
Contributor Author

2.2.0-SNAPSHOT

@dkayiwa
Copy link
Member

dkayiwa commented May 4, 2018

And when you run the setup, did you have all the reference application modules loaded from the very start?

@paradisekelechi
Copy link
Contributor Author

No I did not have them

@dkayiwa
Copy link
Member

dkayiwa commented May 5, 2018

@paradisekelechi can you do that?

@paradisekelechi
Copy link
Contributor Author

@dkayiwa I can do that. I am on it now

@paradisekelechi
Copy link
Contributor Author

I have added the reference modules

@dkayiwa
Copy link
Member

dkayiwa commented May 7, 2018

Now do a fresh setup with those modules installed

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 59.102% when pulling d08f28c on paradisekelechi:TRUNK-5338 into 1d14c59 on openmrs:master.

@samuelmale
Copy link
Member

Are you still working on this @paradisekelechi ?

1 similar comment
@samuelmale
Copy link
Member

Are you still working on this @paradisekelechi ?

@paradisekelechi
Copy link
Contributor Author

I have worked on this. I am waiting for verifications and updates from @dkayiwa

</preConditions>
<comment>
Add condition table
Rename conditions table to emrapi_conditions
Copy link
Member

@wluyima wluyima May 14, 2018

Choose a reason for hiding this comment

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

Why are you renaming the table? I thought you should be updating it to match the new design

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wluyima
We are renaming the table because of some edge cases we saw in the implementation.
Assume we are running a fresh instance of the core platform and the emrapi module.
If we update the conditions table here, the emrapi module tries to create a new conditions table which throws errors.

However, the following implementation handles that edge case successfully. On the core platform, the conditions table is renamed to emrapi_conditions. Then a conditions table is created and data migrated to it. On the emrapi module, the emrapi_conditions table is used.

Copy link
Member

Choose a reason for hiding this comment

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

You should not be renaming the table, in any case it's no longer going to live in the emrapi module so there if no need to add that prefix

Copy link
Member

Choose a reason for hiding this comment

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

If the table exists just do the migration otherwise create it and the name should be and remain conditions

Copy link
Member

Choose a reason for hiding this comment

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

@wluyima , wat do u mean if the table exists , just do the migration ..? i think that will be just do the modification. because migration is posiible only after first renaming it.

@paradisekelechi
Copy link
Contributor Author

I have thoroughly tested this @dkayiwa

@samuelmale
Copy link
Member

samuelmale commented May 31, 2018

Hi @paradisekelechi
Are you still working on this? Is so, did you address all @wluyima's requested changes?
Please look into this and we close this :)

@dkayiwa dkayiwa merged commit dd3d494 into openmrs:master Jun 14, 2018
jtatia pushed a commit to jtatia/openmrs-core that referenced this pull request Aug 20, 2018
JyothsnaAshok pushed a commit to JyothsnaAshok/openmrs-core that referenced this pull request Oct 23, 2018
JyothsnaAshok added a commit to JyothsnaAshok/openmrs-core that referenced this pull request Oct 23, 2018
JyothsnaAshok added a commit to JyothsnaAshok/openmrs-core that referenced this pull request Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants