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

Add photo field to inspector form #2965

Merged
merged 3 commits into from
Aug 6, 2020
Merged

Conversation

chrismytton
Copy link
Member

This adds a photo upload field to the inspector form.

Fixes #2902
Related to #2901

Screenshot 2020-04-17 at 06 12 26

@codecov
Copy link

codecov bot commented Apr 17, 2020

Codecov Report

Merging #2965 into master will decrease coverage by 0.01%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2965      +/-   ##
==========================================
- Coverage   83.50%   83.48%   -0.02%     
==========================================
  Files         250      250              
  Lines       15616    15620       +4     
  Branches     2921     2922       +1     
==========================================
+ Hits        13040    13041       +1     
- Misses       1655     1657       +2     
- Partials      921      922       +1     
Impacted Files Coverage Δ
perllib/FixMyStreet/App/Controller/Report.pm 87.46% <25.00%> (-0.78%) ⬇️

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 994093c...ae687e6. Read the comment docs.

@chrismytton chrismytton requested a review from dracos April 17, 2020 10:04
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.

A few small suggestions is all.

templates/web/base/report/inspect/public_update.html Outdated Show resolved Hide resolved
t/app/controller/report_inspect.t Outdated Show resolved Hide resolved
t/app/controller/report_inspect.t Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Report.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Report.pm Outdated Show resolved Hide resolved
@chrismytton chrismytton force-pushed the add-photo-field-to-inspector-form branch from 2b1d35d to 0828ab6 Compare April 20, 2020 10:37
@chrismytton chrismytton requested a review from dracos April 20, 2020 15:07
@chrismytton
Copy link
Member Author

While demo-ing the inspector tool on staging @dracos noticed that this change actually breaks the offline "save submitted form offline and submit to server when back online" functionality. Looks like that doesn't handle multipart forms correctly (it's assuming it's just a foo=1&bar=2 string).

Also worth noting that formData() is not available in service workers in Safari.

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.

Other than the thing you've just mentioned, changes all look good - I think there's another partial you could have extracted in report/new/form_report.html?

@dracos dracos force-pushed the add-photo-field-to-inspector-form branch from 77902e4 to 809c7c0 Compare May 7, 2020 17:58
@dracos dracos force-pushed the add-photo-field-to-inspector-form branch from a244d66 to eb8f08f Compare June 12, 2020 15:34
@dracos dracos force-pushed the add-photo-field-to-inspector-form branch from eb8f08f to a0d720c Compare July 1, 2020 15:47
@dracos dracos requested a review from struan July 1, 2020 15:47
@dracos dracos force-pushed the add-photo-field-to-inspector-form branch from a0d720c to 85c03e3 Compare July 15, 2020 10:10
@davea davea requested review from davea and removed request for struan July 16, 2020 13:52
Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

The Dropzone widget doesn't load if the report is loaded from the sidebar (e.g. on /reports or /my/planned):

dropzone_ajax_load

If you hit reload or visit the report URL directly it's fine though:

dropzone__reload

(guessing it's not running the Dropzone JS after AJAX loading the report content)

Submitting the form with a photo offline on iOS also results in a "Safari cannot open the page because your iPhone is not connected to the internet" message, which I've not yet been able to track down the cause of. It looks like it's trying to do a POST to /report/<id> instead of being handled by the JS/service worker.

@struan struan requested review from davea and dracos July 24, 2020 10:08
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.

That looks to be the fix for the dropzone issue, certainly. This has been round long enough, so I'd say let's tidy/merge even if there is an iOS issue? I can't see what it would be, though :-/ Could hide form from iOS users if people start using it in anger, and/or revisit if/as we work on more full offline support, I guess.

davea
davea previously requested changes Jul 28, 2020
Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

Agreed with @dracos that we should ship this sooner rather than later. However I don't think we can do that in the current state with the iOS bug as that's a bit of a showstopper for any staff affected. Removing the fields for offline iOS users seems like a good compromise for now.

@davea
Copy link
Member

davea commented Jul 30, 2020

The most recent two commits should sort the iOS issue 👍 @dracos do they look ok?

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 unneeded commit (if I've understood it right) is all! Watch out for touchscreen desktop macs I guess ;-/

Open a new ticket about getting photo upload working offline I guess. Did you get any useful info in debugging it as to why it wasn't being handled by the service worker?

And I guess another ticket for having an offline-only Dropzone that could display the same as normal usage but only set the client input rather than try and upload async.

web/cobrands/fixmystreet/fixmystreet.js Show resolved Hide resolved
@davea davea force-pushed the add-photo-field-to-inspector-form branch from e103dd2 to 647be7e Compare August 6, 2020 08:59
@davea davea dismissed their stale review August 6, 2020 09:03

Can't review my own code

@davea davea requested a review from dracos August 6, 2020 09:37
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 bit I think would be better in the other commit, but meh :) Phewee.

@@ -302,6 +302,14 @@ fixmystreet.offline = (function() {
$('.moderate-display.segmented-control, .shadow-wrap, #update_form, #report-cta, .mysoc-footer, .nav-wrapper').hide();
$('.js-back-to-report-list').attr('href', '/my/planned');

// On iOS we want to hide the photo fields on the offline inspector
Copy link
Member

Choose a reason for hiding this comment

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

Pedantic, but I guess this should be in the following commit, as in this commit, offline support is just broken for everything because the changes in the following commit have not yet been made.

chrismytton and others added 2 commits August 6, 2020 10:59
This adds the code for photo uploads from the regular update form to the
inspector form, and adds details to the documentation.
This manually reconstructs the POST as there is no support for formData
in safari, plus our storage mechanism does not handle formData as it's
not a simple object.
@davea davea force-pushed the add-photo-field-to-inspector-form branch from 647be7e to ae687e6 Compare August 6, 2020 10:05
@davea davea merged commit ae687e6 into master Aug 6, 2020
@github-pages github-pages bot temporarily deployed to github-pages August 6, 2020 11:04 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.

Allow inspectors to post a photo along with their update
4 participants