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

[Bromley] Include notes in updates from Echo #5277

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dracos
Copy link
Member

@dracos dracos commented Nov 27, 2024

And fix issue with being redirected from Echo to Bromley more than once.
Fixes https://github.com/mysociety/societyworks/issues/4633 [skip changelog]

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.37%. Comparing base (eeed092) to head (65a3a6f).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
perllib/FixMyStreet/Cobrand/Bromley.pm 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5277      +/-   ##
==========================================
+ Coverage   82.35%   82.37%   +0.01%     
==========================================
  Files         413      413              
  Lines       32668    32673       +5     
  Branches     5236     5236              
==========================================
+ Hits        26904    26914      +10     
+ Misses       4215     4211       -4     
+ Partials     1549     1548       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dracos dracos force-pushed the sw-4633-bromley-veolia-notes branch from d19e95a to abd10c5 Compare November 27, 2024 13:45
Copy link
Contributor

@nephila-nacrea nephila-nacrea left a comment

Choose a reason for hiding this comment

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

Looks alright to me as far as I can tell. I'm just not very clear on the journey of a report being referred to Bromley from Echo or vice versa.

foreach (@$data) {
$notes = $_->{Value} if $_->{DatatypeName} eq 'Veolia Notes';
}

# An update from Echo with resolution code 1252
my $code = $comment->get_extra_metadata('external_status_code') || '';
if ($code eq '1252') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it would be good to have a comment or constant to explain what code 1252 refers to exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this could certainly do with more description (and on the wiki too, now the project's live). Hopefully the "redirecting of reports between backends" tests in t/cobrand/bromley.t at least show the behaviour of things bouncing back and forth.
(Basically, if a report is sent to Echo, they can refer it to Bromley by sending a '1252' update code to us)

If a report was originally sent to Echo, then referred to Bromley more
than once, we do not want to send the report to Bromley more than once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants