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

Fix email alert on initial update template. #3209

Merged
merged 1 commit into from Oct 9, 2020

Conversation

dracos
Copy link
Member

@dracos dracos commented Oct 8, 2020

A report's confirmation timestamp uses current_timestamp, and so
includes microseconds. An initial update text, to fit in with the
Open311 handling of updates, uses a DateTime object, which does not.
This means if a report is created when logged in, the initial update
can have a timestamp earlier than the report, and so is not alerted on.

@dracos dracos requested a review from struan October 8, 2020 10:34
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #3209 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3209   +/-   ##
=======================================
  Coverage   83.80%   83.80%           
=======================================
  Files         249      249           
  Lines       15803    15803           
  Branches     2970     2970           
=======================================
  Hits        13243    13243           
  Misses       1621     1621           
  Partials      939      939           
Impacted Files Coverage Δ
perllib/FixMyStreet/App/Controller/Report/New.pm 90.96% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76c4765...2d87d6b. Read the comment docs.

@@ -1736,7 +1736,7 @@ sub create_related_things : Private {
my $request = {
service_request_id => $problem->id,
update_id => 'auto-internal',
comment_time => DateTime->now,
comment_time => DateTime->now->add( seconds => 1 ),
Copy link
Member

Choose a reason for hiding this comment

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

I think a comment to indicate what's going on here would be helpful.

A report's confirmation timestamp uses current_timestamp, and so
includes microseconds. An initial update text, to fit in with the
Open311 handling of updates, uses a DateTime object, which does not.
This means if a report is created when logged in, the initial update
can have a timestamp earlier than the report, and so is not alerted on.
@dracos dracos force-pushed the fix-first-comment-timestamp branch from 9296c6a to 2d87d6b Compare October 9, 2020 07:07
@dracos dracos merged commit 2d87d6b into master Oct 9, 2020
@github-pages github-pages bot temporarily deployed to github-pages October 9, 2020 08:52 Inactive
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.

None yet

2 participants