JN-1059 existing user import updates #898

ssettipalli · 2024-05-17T17:33:04Z

DESCRIPTION (include screenshots, and mobile screenshots for participant UX)

While importing check for participant existence and handle as update

    /** while importing handle update for existing import
     if same enrolle: update enrollee
     if same participant & same portal.. new enrollee & same profile
     if same participant & different portal.. new enrollee & new profile
     **/

TO TEST: (simple manual steps for confirming core behavior -- used for pre-release checks)

Use attached heartdemo -v1, v2 files and hearthive v1, v2 files for testing.
load hd-v1; make sure import looks good;
load hd-v2; import should go through and existing enrollees/participants should be updated; same profile

load hh-v1; import should go through; same participants (no new participants should be created; new enrollees should be created with new profile since diff portal;
load hh-v2; hh enrollees should be updated;
Dont have test files for same portal, same participant but different study.

2024-05-17-enrollees-hd-v1.csv
2024-05-17-enrollees-hd-v2.csv
2024-05-17-enrollees-hh-v1.csv
2024-05-17-enrollees-hh-v2.csv

core/src/main/java/bio/terra/pearl/core/service/export/EnrolleeImportService.java

ssettipalli · 2024-05-17T17:37:14Z

core/src/test/java/bio/terra/pearl/core/service/export/EnrolleeImportServiceTests.java

@@ -73,23 +74,73 @@ public class EnrolleeImportServiceTests extends BaseSpringBootTest {
    @Test
    @Transactional
    public void testImportEnrolleesCSV(TestInfo info) {
+        //tests new import and update import


combined new import and then update test cases too.
Maybe they can be separated but didnt wanted to reuse already populated test portal/study env

not sure I agree -- seems like this test is now too long. Could you pull the first part of the method into a helper method "setupImportTest()" And then have one test that tests a single import, and then an separate test that tests updates?

(sampath) separated out into 3 separate tests.

+1 Devon, I don't think these should be combined- it feels like this is pushing past the limits of what could be considered a "unit" test

ssettipalli · 2024-05-17T17:40:07Z

core/src/test/java/bio/terra/pearl/core/service/export/EnrolleeImportServiceTests.java

+        assertThat(userUpdated2.getId(), equalTo(user.getId()));
+        Assertions.assertNotEquals(enrollee.getId(), enrolleeUpdated2.getId());
+        Assertions.assertNotEquals(enrollee.getProfileId(), enrolleeUpdated2.getProfileId());
+


same portal, different study wasn't tested here as of now

devonbush

yup, you were right that the code could use some cleanup -- also consider some more general test helpers -- we'll be writing a lot of tests for this over the next month :)

core/src/main/java/bio/terra/pearl/core/service/export/EnrolleeImportService.java

devonbush · 2024-05-17T21:28:19Z

core/src/test/java/bio/terra/pearl/core/service/export/EnrolleeImportServiceTests.java

+
+    }
+
+    private Import importAndVerify(StudyEnvironmentFactory.StudyEnvironmentBundle bundle, AdminUser admin, String csvString,


It took me a long time to figure out what this method did. Consider renaming, adding a comment, and also renaming the "birthDate" parameter to clarify which birthdate

Better yet, consider splitting out an "Import" helper method, and then a series of "verifyImportEnrollees" "verifyImportParticipantUsers" "verifySurveyResponses" methods. Each method would take as arguments an Import and an array of hamcrest Matchers. this should reduce the amount of hardcoded strings.

Took care of most except "array of hamcrest Matchers" which I will look into separately.

refactored to pass objects rather than individual attributes so that it looks good with more attributes from ParticipantUser/Enrolle/Profile

devonbush

Some test cleanups that would be nice, but not blocking

core/src/test/java/bio/terra/pearl/core/service/export/EnrolleeImportServiceTests.java

devonbush · 2024-05-20T19:02:30Z

core/src/test/java/bio/terra/pearl/core/service/export/EnrolleeImportServiceTests.java

@@ -73,23 +74,73 @@ public class EnrolleeImportServiceTests extends BaseSpringBootTest {
    @Test
    @Transactional
    public void testImportEnrolleesCSV(TestInfo info) {
+        //tests new import and update import


not sure I agree -- seems like this test is now too long. Could you pull the first part of the method into a helper method "setupImportTest()" And then have one test that tests a single import, and then an separate test that tests updates?

(sampath) separated out into 3 separate tests.

core/src/main/java/bio/terra/pearl/core/service/export/EnrolleeImportService.java

MatthewBemis

Looks good pending resolution of latest comments 👍

MatthewBemis · 2024-05-20T21:54:07Z

core/src/test/java/bio/terra/pearl/core/service/export/EnrolleeImportServiceTests.java

@@ -73,23 +74,73 @@ public class EnrolleeImportServiceTests extends BaseSpringBootTest {
    @Test
    @Transactional
    public void testImportEnrolleesCSV(TestInfo info) {
+        //tests new import and update import


+1 Devon, I don't think these should be combined- it feels like this is pushing past the limits of what could be considered a "unit" test

sonarcloud · 2024-05-21T00:42:18Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Sampath K. Settipalli added 8 commits May 17, 2024 06:49

JN-1059 . handle data updates

4842aab

JN-1059 . handle data updates

d3ed52a

JN-1059 . handle data updates

9e51fdc

JN-1059 . handle data updates

ec0fa15

JN-1059 . handle data updates

153bd91

JN-1059 . handle data updates

e0c1cc8

JN-1059 . handle data updates

e9d4308

JN-1059 . handle data updates

ebd83e3

ssettipalli changed the title ~~JN-1059 existing user updates~~ JN-1059 existing user import updates May 17, 2024

ssettipalli commented May 17, 2024

View reviewed changes

core/src/main/java/bio/terra/pearl/core/service/export/EnrolleeImportService.java Outdated Show resolved Hide resolved

ssettipalli commented May 17, 2024

View reviewed changes

JN-1059 . handle data updates

517cd60

ssettipalli marked this pull request as ready for review May 17, 2024 17:52

ssettipalli requested review from devonbush and MatthewBemis May 17, 2024 17:52

devonbush requested changes May 17, 2024

View reviewed changes

Sampath K. Settipalli added 2 commits May 17, 2024 20:57

JN-1059 . handle data updates

0e03ad7

JN-1059 . handle data updates

c2515f2

ssettipalli marked this pull request as draft May 18, 2024 21:02

Sampath K. Settipalli added 4 commits May 18, 2024 19:07

JN-1059 . handle data updates

8dd6b35

JN-1059 . handle data updates

54b6660

cleanup

8ae51cb

cleanup

5edee7e

ssettipalli marked this pull request as ready for review May 20, 2024 02:42

ssettipalli requested a review from devonbush May 20, 2024 02:42

devonbush approved these changes May 20, 2024

View reviewed changes

Sampath K. Settipalli added 2 commits May 20, 2024 14:24

cleanup

8268ebd

cleanup

a8394c0

MatthewBemis approved these changes May 20, 2024

View reviewed changes

Sampath K. Settipalli added 4 commits May 20, 2024 19:16

cleanup

5431950

cleanup

937b964

cleanup

265b8c7

Merge branch 'development' into JN-1059-existing-user-updates

61f6b77

ssettipalli added this pull request to the merge queue May 21, 2024

Merged via the queue into development with commit c5b2698 May 21, 2024
12 checks passed

ssettipalli deleted the JN-1059-existing-user-updates branch May 21, 2024 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JN-1059 existing user import updates #898

JN-1059 existing user import updates #898

ssettipalli commented May 17, 2024 •

edited

ssettipalli May 17, 2024

devonbush May 20, 2024 •

edited by ssettipalli

MatthewBemis May 20, 2024

ssettipalli May 17, 2024

devonbush left a comment

devonbush May 17, 2024

devonbush May 17, 2024 •

edited

ssettipalli May 20, 2024

ssettipalli May 21, 2024 •

edited

devonbush left a comment

devonbush May 20, 2024 •

edited by ssettipalli

MatthewBemis left a comment

MatthewBemis May 20, 2024

sonarcloud bot commented May 21, 2024


		}

		private Import importAndVerify(StudyEnvironmentFactory.StudyEnvironmentBundle bundle, AdminUser admin, String csvString,

JN-1059 existing user import updates #898

JN-1059 existing user import updates #898

Conversation

ssettipalli commented May 17, 2024 • edited

DESCRIPTION (include screenshots, and mobile screenshots for participant UX)

TO TEST: (simple manual steps for confirming core behavior -- used for pre-release checks)

ssettipalli May 17, 2024

Choose a reason for hiding this comment

devonbush May 20, 2024 • edited by ssettipalli

Choose a reason for hiding this comment

MatthewBemis May 20, 2024

Choose a reason for hiding this comment

ssettipalli May 17, 2024

Choose a reason for hiding this comment

devonbush left a comment

Choose a reason for hiding this comment

devonbush May 17, 2024

Choose a reason for hiding this comment

devonbush May 17, 2024 • edited

Choose a reason for hiding this comment

ssettipalli May 20, 2024

Choose a reason for hiding this comment

ssettipalli May 21, 2024 • edited

Choose a reason for hiding this comment

devonbush left a comment

Choose a reason for hiding this comment

devonbush May 20, 2024 • edited by ssettipalli

Choose a reason for hiding this comment

MatthewBemis left a comment

Choose a reason for hiding this comment

MatthewBemis May 20, 2024

Choose a reason for hiding this comment

sonarcloud bot commented May 21, 2024

Quality Gate passed

ssettipalli commented May 17, 2024 •

edited

devonbush May 20, 2024 •

edited by ssettipalli

devonbush May 17, 2024 •

edited

ssettipalli May 21, 2024 •

edited

devonbush May 20, 2024 •

edited by ssettipalli