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.

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

Remove non-word characters from event UIDs #784

Merged
merged 1 commit into from Feb 13, 2020
Merged

Conversation

@odub
Copy link
Contributor

odub commented Feb 12, 2020

Hey,

Thanks again for your work on the bank holiday calendar UID update.

This is a simple piece of defensive programming after observing this UID in the latest production .ics output:

BEGIN:VEVENT
DTEND;VALUE=DATE:20150102
DTSTART;VALUE=DATE:20150101
SUMMARY:New Year’s Day
UID:ca6af7456b0088abad9a69f9f620f5ac-2015-01-01-NewYear’sDay@gov.uk
SEQUENCE:0
DTSTAMP:20200210T141037Z
END:VEVENT

After my proposed change the UID line will come out as:

UID:ca6af7456b0088abad9a69f9f620f5ac-2015-01-01-NewYearsDay@gov.uk

Removing non-word characters seemed like the best way to avoid accounting for the text escaping rules in the ICS format spec. Without this change, an event name with a : or ; or , in could break an otherwise valid ICS file.

Hope this helps!

@cbaines

This comment has been minimized.

Copy link
Contributor

cbaines commented Feb 13, 2020

That looks good to me. Thanks for taking a look.

I've pushed your commits to this repository as a branch, so that Jenkins can run the expected checks.

@odub

This comment has been minimized.

Copy link
Contributor Author

odub commented Feb 13, 2020

Sounds smart, I think I see that here

@odub

This comment has been minimized.

Copy link
Contributor Author

odub commented Feb 13, 2020

If you want me to close this and open a PR from that branch let me know. (Alternatively if you know a way to switch the "head" branch of an open PR I'd be interested to hear)

@cbaines

This comment has been minimized.

Copy link
Contributor

cbaines commented Feb 13, 2020

If you want me to close this and open a PR from that branch let me know. (Alternatively if you know a way to switch the "head" branch of an open PR I'd be interested to hear)

This Pull Request is fine. The checks are tracked by commit, so the branch doesn't matter.

@odub

This comment has been minimized.

Copy link
Contributor Author

odub commented Feb 13, 2020

Copy link
Contributor

cbaines left a comment

🥇 Looks good to me 👍

@issyl0
issyl0 approved these changes Feb 13, 2020
Copy link
Member

issyl0 left a comment

Thanks for your contribution! 🥇

@issyl0 issyl0 merged commit f8ae65c into alphagov:master Feb 13, 2020
3 checks passed
3 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
continuous-integration/jenkins/security No security issues found
Details
@issyl0

This comment has been minimized.

Copy link
Member

issyl0 commented Feb 13, 2020

This change has been deployed to production. Thanks again, @odub! 👏

@odub

This comment has been minimized.

Copy link
Contributor Author

odub commented Feb 13, 2020

Cheers! Thank you

@odub odub deleted the odub:ensure-uid-text-format branch Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.