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-4202] Convert Order.orderer to be a Provider #563

Closed
wants to merge 4 commits into from

Conversation

k-joseph
Copy link
Member

https://tickets.openmrs.org/browse/TRUNK-4202 | created provider accounts for orderer s, migrated orderer s to be provider ids instead of user ids

ON o.orderer = u.user_id INNER JOIN provider p ON p.person_id = u.person_id) AS b ON o.orderer = b.orderer
SET o.orderer = b.provider_id
</sql>
</changeSet>

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a unit test to Database1_9To1_10UpgradeTest to ensure that this works as expected

Copy link
Member Author

Choose a reason for hiding this comment

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

@wluyima : i have checked whether i could see any documentation inform of parhaps wiki pages about; "unit testing for our changesets" and i didn't get one, it looks to me like this is still under development as can be seen @{https://tickets.openmrs.org/browse/TRUNK-4052}, in case am not right in this manner, then i request to be directed to that way in which i can have my changeset tested according to openmrs standards

Copy link
Member

Choose a reason for hiding this comment

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

See Database1_9To1_10UpgradeTest.java and add your tests to it

Copy link
Member Author

Choose a reason for hiding this comment

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

am challenged to write a changeset that; shouldMigrateDataInOrdererColumnToBeProviderIdsInsteadOfUserIds, since am not sure of which way to use in order to run my changeset inside the testcase to update the datasets available within openmrs now that this is under development

Copy link
Member

Choose a reason for hiding this comment

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

Look at the tests in that class, yours will be very similar


<changeSet id="201401161532-TRUNK-4202" author="k-joseph" dbms="mysql">
<preConditions onFail="MARK_RAN">
<columnExists tableName="orders" columnName="orderer" />
Copy link
Member

Choose a reason for hiding this comment

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

This columnExists is not necessary

@k-joseph
Copy link
Member Author

k-joseph commented Feb 1, 2014

here is my query;
INSERT INTO provider(person_id, identifier, creator, date_created, retired, uuid)
SELECT u.person_id, u.system_id , (select user_id from users where system_id = "daemon"), CURRENT_TIMESTAMP , u.retired, UUID() FROM users as u JOIN orders AS o ON o.orderer = u.user_id WHERE u.person_id NOT IN (SELECT DISTINCT person_id FROM provider)

the other query looks also like;
UPDATE orders AS o INNER JOIN (SELECT o.orderer, p.provider_id FROM orders o INNER JOIN users u ON o.orderer = u.user_id INNER JOIN provider p ON p.person_id = u.person_id) AS b ON o.orderer = b.orderer SET o.orderer = b.provider_id

i think i got some mismatches within the local as compared to the origin branch, which i tried to resolve by pulling first from upstream and then also from origin, which after doing i tried to push i had to resolve by resolving some conflicts, this i did successfully and unfortunately, this resulted into some failure when trying to push some changes to the origin branch, here is the failure trace;
To git@github.com:k-joseph/openmrs-core.git
! [rejected] TRUNK-4202 -> TRUNK-4202 (non-fast-forward)
error: failed to push some refs to 'git@github.com:k-joseph/openmrs-core.git'
To prevent you from losing history, non-fast-forward updates were rejected
Merge the remote changes (e.g. 'git pull') before pushing again. See the
'Note about fast-forwards' section of 'git push --help' for details.

so as am still clearing the issues in the queries i kindly ask better review of them by devs as @wluyima here so that i can fully clear up the failures in the test, otherwise am trying to resolve this push rejected failure issue, regards

@wluyima
Copy link
Member

wluyima commented Feb 1, 2014

The query that updates orderer i think can written in a cleaner and easier way to understand by just joining directly to the provider and user tables without that subselect which makes things a little harder to read.
Did you remove the @ignore from the test method i added and make sure it is passing?

About the push, you must have rebased something you had already pushed to the 1.10.x branch in your fork and that is why git is rejecting the push to your fork, you can add -f when pushing to force the push to your fork

@wluyima
Copy link
Member

wluyima commented Feb 1, 2014

And in the INSERT query, when selecting the user_id for daemon, can you use the uuid in the WHERE clause instead of the system_id(deamon), it is more reliable to use the uuid

@k-joseph
Copy link
Member Author

k-joseph commented Feb 2, 2014

i have resolved the uuid issue. and the query is;

INSERT INTO provider(person_id, identifier, creator, date_created, retired, uuid)
SELECT u.person_id, u.system_id , (select user_id from users where uuid = "A4F30A1B-5EB9-11DF-A648-37A07F9C90FB"), CURRENT_TIMESTAMP , u.retired, UUID() FROM users as u JOIN orders AS o ON o.orderer = u.user_id
WHERE u.person_id NOT IN (SELECT DISTINCT person_id FROM provider)

and then the other query is;

UPDATE orders AS o INNER JOIN provider p ON o.orderer = p.provider_id
INNER JOIN users u ON o.orderer = u.user_id AND o.orderer = p.provider_id
SET o.orderer = p.provider_id

however the test is still failing with the same message as before, any other thing i need to adjust should be pointed out here so that i can clear it as well and have the test pass, otherwise as @wluyima suggests am looking forward to make sure that the test passes however i agree that my sql knowledge is just being improved and that is partly the reason why am even taking alot of time on this issus than was originally planned and expected, sorry for any inconveniences this delay has caused,
i call upon any other devs to take part in helping me improve these queries to fit what is expected, thanks

@wluyima
Copy link
Member

wluyima commented Feb 3, 2014

No problem, we are all always learning new things and keep improving.
The query is still incorrect because you are joining wrongly to the providers table, you need to always remember that orders.orderer is a fk to users.user_id prior to 1.10, so you should join first to the users table and then join users to provider, the orders table before 1.10 has no fk to providers and the ticket is about making this change to so that orders.orderer is a provider

@wluyima
Copy link
Member

wluyima commented Feb 3, 2014

We can pair later today if you still have issues

@wluyima
Copy link
Member

wluyima commented Feb 6, 2014

Merged at 0ea76fa

@wluyima wluyima closed this Feb 6, 2014
RandilaP pushed a commit to RandilaP/openmrs-core that referenced this pull request Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants