Skip to content

Conversation

@jasonleenaylor
Copy link
Contributor

@jasonleenaylor jasonleenaylor commented Jul 23, 2025

  • Store the date at the start of the import and use it in entry creation. We need to have a consistent date recorded to determine if we read a date from the xml data or not.
  • Add a function to return the newest date that was actually read from the xml data.

This change is Reviewable

* Store the date at the start of the import and use it in entry
  creation. We need to have a consistent date recorded to determine
  if we read a date from the xml data or not.
* Add a function to return the newest date that was actually read
  from the xml data.
@github-actions
Copy link

github-actions bot commented Jul 23, 2025

LCM Tests

    16 files  ±0      16 suites  ±0   2m 59s ⏱️ +2s
 2 836 tests +1   2 816 ✅ +1   20 💤 ±0  0 ❌ ±0 
11 292 runs  +4  11 124 ✅ +4  168 💤 ±0  0 ❌ ±0 

Results for commit 913b234. ± Comparison against base commit 7120fad.

♻️ This comment has been updated with latest results.

Copy link

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


src/SIL.LCModel/Application/ApplicationServices/XmlImportData.cs line 491 at r1 (raw file):

				return;
			}
			if (leNew.DateCreated == m_importDate)

Would it be worth having >= m_importDate on these two checks just in case a .DateCreated is from DateTime.Now on a system with a different clock?

@jasonleenaylor
Copy link
Contributor Author

src/SIL.LCModel/Application/ApplicationServices/XmlImportData.cs line 491 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Would it be worth having >= m_importDate on these two checks just in case a .DateCreated is from DateTime.Now on a system with a different clock?

No, I don't think it is worth it. The real concern here is retaining data coming in from the import, if we end up with created dates in the future that is a GIGO issue for the user that doesn't cause any real issues for our functionality.

@jasonleenaylor jasonleenaylor enabled auto-merge (squash) July 24, 2025 18:44
@jasonleenaylor jasonleenaylor force-pushed the bugfix/LT-21976 branch 2 times, most recently from c23944e to b2946b0 Compare July 24, 2025 19:00
Copy link

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

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

:lgtm: (but I don't have "Approve" permissions)

Reviewed 1 of 2 files at r1, 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jasonleenaylor)


tests/SIL.LCModel.Tests/Application/ApplicationServices/XmlImportDataTests.cs line 856 at r6 (raw file):

				"</FwDatabase>" + Environment.NewLine
				))
				using(var writer = new StringWriter(sbLog))

(found it!)

@jasonleenaylor jasonleenaylor merged commit a8f8ecf into master Jul 24, 2025
4 checks passed
@jasonleenaylor jasonleenaylor deleted the bugfix/LT-21976 branch July 24, 2025 19:48
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.

3 participants