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

RESTWS-741: Remove 1.8 submodule (Removed 0mod-1.8Controller) #619

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

josephbate
Copy link

Description of what I changed

  1. Transfered controller classes and dependant classes from 1.8 to 1.9
  2. Edited RestTestConstants1_9.java to include all the constants of RestTestConstants1_8.java
  3. edited all classes in the omods above 1.8 to import RestTestConstants1_9.java
  4. transfered resourse datasets from 1.8 to 1.9

Cc. @dkayiwa @ibacher @mks-d

Issue I worked on

see https://openmrs.atlassian.net/browse/RESTWS-741

Checklist: I completed these to help reviewers :)

  • 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

@ibacher
Copy link
Member

ibacher commented Aug 20, 2024

Also, it doesn't look like this removes the whole submodule?

@josephbate
Copy link
Author

josephbate commented Aug 20, 2024

Also, it doesn't look like this removes the whole submodule?

true i am doing it in phases
it helps me keep track and easily spot failures.
next is resources
just having power issues over

@ibacher
Copy link
Member

ibacher commented Aug 20, 2024

Ah ok! Do you mind if I convert this to a draft then? (I like the idea of doing this in stages!)

@ibacher ibacher marked this pull request as draft August 20, 2024 18:47
@josephbate
Copy link
Author

Ah ok! Do you mind if I convert this to a draft then? (I like the idea of doing this in stages!)

It's fine with
Should I continue to push more commits on this same pr or should i make separate ones for each commit?

@ibacher
Copy link
Member

ibacher commented Aug 20, 2024

Let's keep it all in one PR, if that's alright...

@josephbate
Copy link
Author

Let's keep it all in one PR, if that's alright...

Alright then
Thank you @ibacher

@josephbate
Copy link
Author

josephbate commented Sep 6, 2024

hello @ibacher
here is an update on this ticket , i have managed to migrate the resource classes to from omod 1.8 to 1.9
however i ran into trouble with some test classes which cause test failures;
following up on the talk thread Testcase failures

  1. on PersonAttributeResource1_9Test() should we ignore the setValue_shouldSetProperAttributableIdIfFound() or just modify the expected result to match the actual result
  2. on PersonAttributeTypeResourceWithConcept1_9Test() please look into validateFullRepresentation() because inspite of the correct approach to cast an instance of SimpleObject to an org.openmrs.Concept it still throws a ClassCastException
  3. i have commented out both of this test cases as i wait for your thoughts on these two issues

Cc. @dkayiwa @mks-d

@ibacher
Copy link
Member

ibacher commented Sep 10, 2024

I've fixed the tests so they work (according to me).

@josephbate
Copy link
Author

josephbate commented Sep 10, 2024

I've fixed the tests so they work (according to me).

thank you for this fix
it also makes sense to for

I've fixed the tests so they work (according to me).

it makes sense with me too

let me these changes update my repository and send my last commit
iam gonna make a new pr if you don't mind @ibacher

@josephbate
Copy link
Author

hello @ibacher
i have created a new pr

see #624

please look into it i think its ready for mergeing

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