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.

Sign up for GitHub

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

Jump to bottom

Ensure map icon URLs are correctly generated with fingerprints #5635

Merged
merged 6 commits into from
Jun 18, 2020
Merged

Ensure map icon URLs are correctly generated with fingerprints #5635

merged 6 commits into from
Jun 18, 2020

Conversation

Matt-Yorkley
Copy link
Contributor

What? Why?

Closes #5633

Closely related to #5622

Ensures map icon URLs are generated with fingerprints (and correctly found).

What should we test?

Maps icons are all okay in v3.

Release notes

Fixed missing map icons issue in Rails 4.

@Matt-Yorkley Matt-Yorkley self-assigned this Jun 18, 2020
@Matt-Yorkley Matt-Yorkley marked this pull request as ready for review June 18, 2020 10:41
@Matt-Yorkley Matt-Yorkley added the pr-staged-au staging.openfoodnetwork.org.au label Jun 18, 2020
@Matt-Yorkley
Copy link
Contributor Author

Hmmm... nope, this will be a lot more tricky to fix than I first thought.

sauloperez
sauloperez reviewed Jun 18, 2020
View reviewed changes
Copy link
Contributor

@sauloperez sauloperez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

but shouldn't we then add sprockets-rails?

app/serializers/api/cached_enterprise_serializer.rb Outdated Show resolved Hide resolved
@Matt-Yorkley
Copy link
Contributor Author

but shouldn't we then add sprockets-rails?

I think that initial code comment was a bit off. It looks like asset handling has been extracted a bit, and gets moved to a gem in a later version. Updated.

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Jun 18, 2020 •
edited
Loading

Before and after (Aus Staging):

Screenshot from 2020-06-18 12-55-15

Screenshot from 2020-06-18 14-39-23

"+1" icon is visible, fingerprints are added to all map icon URLs.

luisramos0
luisramos0 approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

It's great you are finding the fixes for these things Matt!

I dont want to block this fix and the release but I'd prefer to see these solutions improved.

app/assets/javascripts/darkswarm/services/map_configuration.js.erb.coffee Show resolved Hide resolved
app/helpers/serializer_helper.rb Outdated Show resolved Hide resolved
luisramos0
luisramos0 reviewed Jun 18, 2020
View reviewed changes
app/services/image_path_service.rb Outdated
@@ -0,0 +1,10 @@
# frozen_string_literal: true

class ImagePathService
Copy link
Contributor

Choose a reason for hiding this comment

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

We agreed we dont use Service in the services names: https://community.openfoodnetwork.org/t/how-to-name-a-new-service-in-ofn/1530/9
We dont need to be strict but we can easily follow the convention here with a service name like ImagePathBuilder.call or just ImagePath.call

app/helpers/serializer_helper.rb Outdated Show resolved Hide resolved
luisramos0
luisramos0 approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

Thanks Matt! I think a service is always better than an helper 👍
I left a comment about the service name.

@Matt-Yorkley
Copy link
Contributor Author

@luisramos0 how's that? 130f639

luisramos0
luisramos0 approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jun 18, 2020
@filipefurtad0
Copy link
Contributor

Hey @Matt-Yorkley ,

In staging AU, the previously missing enterprises are now marked with a +1:
image

In staging AU, the previously missing enterprises in Manchester and Sheffield are as well now marked with a +1:
image

I also checked whether Groups are displayed on maps correctly - all good as well.

Awesome Matt - thank you for tackling this so fast!
Ready to go.

@filipefurtad0 filipefurtad0 removed pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-au staging.openfoodnetwork.org.au labels Jun 18, 2020
@luisramos0
Copy link
Contributor

yeah, amazing!

@luisramos0 luisramos0 merged commit 208623b into openfoodfoundation:master Jun 18, 2020
@Matt-Yorkley Matt-Yorkley mentioned this pull request Jun 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luisramos0 luisramos0 luisramos0 approved these changes

@sauloperez sauloperez sauloperez approved these changes

Assignees

@Matt-Yorkley Matt-Yorkley

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ghost icons on maps
4 participants
@Matt-Yorkley @filipefurtad0 @luisramos0 @sauloperez