Have GetAge() return -1 if Year is null, and fix return type #6888
Conversation
grayeul
requested review from
DawoudIO,
MrClever and
DAcodedBEAT
as code owners
|
(Probably not for this PR but) at some point, we should probably consolidate on the Family model used throughout the codebase instead of using that and separate one that's just for this page. |
|
It looks like there are a lot of end to end test failures, and it's not the normal flaky offenders - can you look into what's going on there? |
|
I'm still trying to get where I understand how the end-to-end tests are working, and how to run them on my own. Not there yet. And I really can't see how the change I made should have caused the kinds of errors I see reported in the Package and Build test... and knowing we have had some repeatability issues before... I'm hoping it is related to that -- though I'd love to be able to follow how to do the individual steps and track down the problem. |
…rthdate-if-not-all-records-have-birthdate
This is what I was instructed with when I first started contributing here: #6554 (comment) I then re-summarized it in the repo itself: /~https://github.com/ChurchCRM/CRM/tree/master/docker#requirements-1 The repeatability issues have historically only been around report generation pages (in that it is flaky and works sometimes) because the testing framework isn't designed to test the exact workflow that we are doing, so it shouldn't be related since this seems more far-reaching. If you hit issues, let's try to figure it out and update the docs from there! |
…rthdate-if-not-all-records-have-birthdate
…ids to selectors used for import field assignments
| 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}); |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm away from my test machine but I wonder if we need to scrollIntoView before selecting
There was a problem hiding this comment.
(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)
There was a problem hiding this comment.
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.
…rthdate-if-not-all-records-have-birthdate
DAcodedBEAT
deleted the
6887-bug-csvimport-fails-when-importing-birthdate-if-not-all-records-have-birthdate
branch
Description & Issue number it closes
Closes #6887 - Adds return -1 to GetAge() if Year is null
Screenshots (if appropriate)
N/A
How to test the changes?
Perform CSVImport on a small file, with some records having birthdates, and others missing. Be sure to map the correct field to the birthdate field. It should fail before this change, and import successfully after this change.
Type of change
How Has This Been Tested?
Tested as described above, with simple 2-record csv file.
Checklist: