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.

Sign up for GitHub

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

Have GetAge() return -1 if Year is null, and fix return type #6888

Merged
merged 11 commits into from
Feb 29, 2024
Merged

Have GetAge() return -1 if Year is null, and fix return type #6888

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
681391e
Have GetAge() return -1 if Year is null, and fix return type
grayeul Feb 21, 2024
1f38ba5
Add arg types and changing comparison on year to ===
grayeul Feb 21, 2024
6720338
Mark Year as nullable
grayeul Feb 21, 2024
879696c
Merge branch 'master' into 6887-bug-csvimport-fails-when-importing-bi…
grayeul Feb 21, 2024
0a4b404
Merge branch 'master' into 6887-bug-csvimport-fails-when-importing-bi…
grayeul Feb 22, 2024
57b0cbf
Update CSVImport to use SESSION to hold csvimport filename, also add …
grayeul Feb 23, 2024
700c93c
Add new test for CSV import, and sample data to be used
grayeul Feb 23, 2024
b4cba13
Add record with Birthday, but no year, to verify can import only Mont…
grayeul Feb 24, 2024
4d9dfe2
Add explicit id to fields needed for testing
grayeul Feb 26, 2024
2992ed9
Reference specific DOM ids for elements needed
grayeul Feb 26, 2024
1c016ad
Merge branch 'master' into 6887-bug-csvimport-fails-when-importing-bi…
grayeul Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cypress/data/test_import.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
ImportTest,hasBday,"123 Main St", Downtown, TX, 77777,myBday@example.com,2001-07-04,888-555-1212
ImportTest,noYrBday,"123 Main St", Downtown, TX, 77777,noYrBday@example.com,0000-07-04,888-555-1212
ImportTest,noBday,"123 Main St", Downtown, TX, 77777,noBday@example.com,,
21 changes: 21 additions & 0 deletions cypress/e2e/ui/admin/admin.csvimport.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
context("CSVImport", () => {
it("Verify CSV Import", () => {
cy.loginAdmin("CSVImport.php");
cy.get("input[type='file']").selectFile('cypress/data/test_import.csv')
cy.get("input[type='submit']").click();
cy.contains('Total number of rows in the CSV file:3');
cy.get('#SelField0').select("Last Name",{force:true});
Copy link
Contributor

Choose a reason for hiding this comment

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

@grayeul is force: true necessary here? https://docs.cypress.io/api/commands/select#Force-select seems to indicate that this is only useful for selecting hidden or disabled options, which shouldn't be the case here.

If it is required (and we ensured that we can't avoid it), we should add a comment to explain why we did this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love for someone else to be able to verify that. I couldn't get the select 'action' to work and tried multiple different ways, finally I added the force:true (which I saw in a few other tests) and then it worked! I agree that shouldn't have been necessary, as the fields were not hidden -- but that was my experience. I can add a comment for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm away from my test machine but I wonder if we need to scrollIntoView before selecting

Copy link
Contributor

@DAcodedBEAT DAcodedBEAT Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(Once we either get rid of the force or add a comment for why we did this, even if it's "it only worked when I did this", this PR should be good to merge)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that was it... The screen is short, and it looked fine when I used the cypress gui, but still didn't select. I think it should have been in view from the start.

cy.get('#SelField1').select("First Name",{force:true});
cy.get('#SelField2').select("Address 1",{force:true});
cy.get('#SelField3').select("City",{force:true});
cy.get('#SelField4').select("State",{force:true});
cy.get('#SelField5').select("Zip",{force:true});
cy.get('#SelField6').select("Email",{force:true});
cy.get('#SelField7').select("Birth Date",{force:true});
cy.get('#SelField8').select("Home Phone",{force:true});
// Now that we have mapped the right fields, do the import
cy.get("input[type='submit']").click();
cy.contains('Data import successful.');
});
});

24 changes: 11 additions & 13 deletions src/CSVImport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,9 @@ public function assignRoles()
$csvError = gettext('No file selected for upload.');
} else {
// Valid file, so save it and display the import mapping form.
$csvTempFile = 'import.csv';
$system_temp = ini_get('session.save_path');
if (strlen($system_temp) > 0) {
$csvTempFile = $system_temp . '/' . $csvTempFile;
}
// Use a temp filename in the system temp dir, and save in SESSION
$csvTempFile = tempnam(sys_get_temp_dir(), 'csvimport');
$_SESSION['csvTempFile'] = $csvTempFile;
move_uploaded_file($_FILES['CSVfile']['tmp_name'], $csvTempFile);

// create the file pointer
Expand Down Expand Up @@ -196,10 +194,11 @@ public function assignRoles()
}

// add select boxes for import destination mapping
// and provide with unique id to assist with testing
for ($col = 0; $col < $numCol; $col++) {
?>
<td>
<select name="<?= 'col' . $col ?>" class="columns">
<select id="<?= 'SelField' . $col ?>" name="<?= 'col' . $col ?>" class="columns">
<option value="0"><?= gettext('Ignore this Field') ?></option>
<option value="1"><?= gettext('Title') ?></option>
<option value="2"><?= gettext('First Name') ?></option>
Expand Down Expand Up @@ -288,14 +287,10 @@ public function assignRoles()
$bHasCustom = false;
$bHasFamCustom = false;

$csvTempFile = 'import.csv';
$system_temp = ini_get('session.save_path');
if (strlen($system_temp) > 0) {
$csvTempFile = $system_temp . '/' . $csvTempFile;
}
//Get the temp filename stored in the session
$csvTempFile = $_SESSION['csvTempFile'];

$Families = [];

// make sure the file still exists
if (file_exists($csvTempFile)) {
// create the file pointer
Expand Down Expand Up @@ -916,8 +911,11 @@ function ParseDate(string $sDate, int $iDateMode): array
return $aDate;
}

function GetAge($Month, $Day, $Year): bool
function GetAge(int $Month, int $Day, ?int $Year): int
{
if ($Year === null) {
return -1;
}
if ($Year > 0) {
if ($Year == date('Y')) {
return 0;
Expand Down
Loading