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

[Brent] Allow reporting of missed small items if overdue #4736

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

Conversation

MorayMySoc
Copy link
Contributor

Allow open small items events through and check if they are overdue. If they are allow them to be reported as missed.

https://mysocietysupport.freshdesk.com/a/tickets/3500

[skip changelog]

Allow open small items events through and check if they are
overdue. If they are allow them to be reported as missed.

https://mysocietysupport.freshdesk.com/a/tickets/3500
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4209729) 85.19% compared to head (7162aaa) 85.95%.
Report is 46 commits behind head on master.

Files Patch % Lines
perllib/FixMyStreet/Cobrand/Brent.pm 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4736      +/-   ##
==========================================
+ Coverage   85.19%   85.95%   +0.75%     
==========================================
  Files         339      340       +1     
  Lines       24149    26764    +2615     
  Branches     4547     5354     +807     
==========================================
+ Hits        20574    23005    +2431     
- Misses       2193     2281      +88     
- Partials     1382     1478      +96     

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

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

Couple of issues, I think, but general looks okay.
I think however (after this PR) it will need adapting to cope with more than one bulky event being found...

my $report = $self->problems->search({ external_id => $_->{Guid} })->first;
$events->{enquiry}->{$event_type} = {
report => $report,
date => construct_bin_date($_->{DueDate}),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we can trust DueDate is the date of the collection, I'm not sure that's documented anywhere. Can we read it from $report->get_extra_field_value('CollectionDate') instead?

perllib/FixMyStreet/Cobrand/Brent.pm Show resolved Hide resolved
Clarify that construct_bin_date requires a hash ref and
return if hash ref doesn't contain required key
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

One query to ask Brent about, I guess.
Not necessarily wanting to expand this, but from K&S work I see that this code won't work if someone has more than one bulky collection active - we're only ever storing one such here. Do you think it would be much work to have it store multiple if it finds them, and use them?

my $report = $self->problems->search({ external_id => $_->{Guid} })->first;
$events->{enquiry}->{$event_type} = {
report => $report,
date => construct_bin_date({ DateTime => $report->get_extra_field_value('Collection_Date')}),
Copy link
Member

Choose a reason for hiding this comment

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

We may fetch an event with this API call that doesn't exist in our system (if it was created outside our system), so I guess in that case we shouldn't set this object at all, as we won't know the collection date then. (Also, we could ask Brent if there is a field on a bulky collection event that is the collection date, and then use that (perhaps I should have suggested asking that in my last comment).)

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