Skip to content

VED-1147: Convert Upstream Time Format to ISO#1327

Merged
Akol125 merged 17 commits intomasterfrom
VED-1147-Time-Format-RFC3339
Mar 26, 2026
Merged

VED-1147: Convert Upstream Time Format to ISO#1327
Akol125 merged 17 commits intomasterfrom
VED-1147-Time-Format-RFC3339

Conversation

@Akol125
Copy link
Copy Markdown
Contributor

@Akol125 Akol125 commented Mar 24, 2026

Summary

  • Routine Change
    Fix a bug for time format crafted in MNS Payload, the time previously used maintained Delta time format rather than converting the format to RFC33399

Add any other relevant notes or explanations here. Remove this line if you have nothing to add.

Reviews Required

  • Dev
  • Test
  • Tech Author
  • Product Owner

Review Checklist

ℹ️ This section is to be filled in by the reviewer.

  • I have reviewed the changes in this PR and they fill all of the acceptance criteria of the ticket.
  • If there were infrastructure, operational, or build changes, I have made sure there is sufficient evidence that the changes will work.
  • If there were changes that are outside of the regular release processes e.g. account infrastructure to setup, manual setup for external API integrations, secrets to set, then I have checked that the developer has flagged this to the Tech Lead as release steps.
  • I have checked that no Personal Identifiable Data (PID) is logged as part of the changes.

@github-actions
Copy link
Copy Markdown
Contributor

This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket:

VED-1147



def _parse_timestamp_to_iso(timestamp: str) -> str:
dt_part = timestamp[:15]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we are assuming an exact shape by slicing fixed positions and then calling [int(timestamp[15:])] It has no explicit validation for:

empty or non-string input
wrong length
missing T
invalid date/time values
unsupported timezone suffixes

Also, I would want a ValueError message not

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could re-check, but this validation are already handled in the delta and API code, making it impossible to have an empty data or incomplete timestamp. Also our occurrence date time would always be in that format YYYYTTHHMDDSfz as described in our OAS, anyone not giving us that format should be rejected upfront in the API.
image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's fair. I think the right place for this helper function should be in the shared folder and so it should be assumed that no validation would be happening on consumers of it. If there additional validation then great but otherwise we should be making it robust. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Having it robust wouldn't do no harm, would add that now


return gp_ods_code


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it better to move this to a shared helper - shared folder perhaps?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We already need to convert from the custom CSV date format to a standard ISO date in recordprocessor. We could share utils_for_fhir_conversion.Convert to save duplicating the code and tests.


self.assertEqual(str(context.exception), "PDS API error")


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

-Please can you add the following:

  • Add a UTC conversion test for 20260212T17443700 -> 2026-02-12T17:44:37Z.

  • Add a BST conversion test for 20260212T17443701 -> 2026-02-12T17:44:37+01:00.

  • Add failure tests for missing, malformed, and unsupported-offset DATE_AND_TIME

  • Add a contract test proving output is RFC 3339 second-precision, not the raw compact string.

Copy link
Copy Markdown
Contributor

@avshetty1980 avshetty1980 left a comment

Choose a reason for hiding this comment

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

just a few comments.
Please can you change the PR title to something like "Convert MNS timestamp in publish payload to RFC3339 format"?

@Thomas-Boyle Thomas-Boyle added the bugfix This PR contains a fix for existing functionality. label Mar 25, 2026
return gp_ods_code


def _parse_timestamp_to_iso(timestamp: str) -> str:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_parse_timestamp_to_iso() has no input shape validation and will raise generic exceptions (TypeError/ValueError) without a clear DATE_AND_TIME contract message. (As Akshay mentioned)

@avshetty1980 avshetty1980 removed the blocked This PR is blocked. label Mar 25, 2026
raise ValueError(f"{field_name} must contain a valid compact timestamp") from e

tz = timezone(timedelta(hours=int(offset_suffix)))
return parsed_date.replace(tzinfo=tz).isoformat(timespec="milliseconds").replace("+00:00", "Z")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we might need to change "milliseconds" to "seconds"

Comment thread tests/e2e_automation/utilities/date_helper.py
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@avshetty1980 avshetty1980 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you Akin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This PR contains a fix for existing functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants