Feature/brave rewards ui rebase2 #1162

yachtcaptain23 · 2018-09-11T23:02:23Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Tagged reviewers.
Integrated piwik/matomo (for code that adds new buttons).
Addressed or ignored all brakeman warnings

Test Plan:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions

Security:

No raw SQL -- Always prefer ActiveRecord query helpers (more info on StackOverflow)
XSS is mitigated -- Avoid html_safe and raw; escape untrusted content from users and 3rd party APIs (Rails XSS guide and OWASP XSS Prevention Cheat Sheet)

nvonpentz

Looking pretty good so far. A few comments besides the inlines in no particular order:

We should add BRAVE_REWARDS_EMAIL_WHITELIST to base.yml so it can be used with docker
I think we should add at least add tests for the Publishers::SiteBannersController as we discussed
The "Instant Donation" modal does not exactly meet the spec in terms of spacing, font size, and buttons.
Spec:
This Pr:
When you click "Instant Donation" then "Preview Banner" then "Close" then "Instant Donation" again, the modal appears offset to the left
Hovering over the photo icons do not produce the pointer cursor that you'd expect
It's not intuitive how to edit the donation amounts
The 5 BAT donation amount is pre highlighted, though it has no significance to the publisher which I found confusing
Donation amounts do not update until after the user clicks "Save Change" AND refreshes the page
There is no way to go from "preview" to "edit", but there is the other way around.

nvonpentz · 2018-09-17T16:34:23Z

app/controllers/publishers/site_banners_controller.rb

+      content_type = "image/png"
+      extension = ".png"
+    else
+      # TODO: Throw an exception here


Should we resolve this TODO?

Going to track in https://github.com/brave-intl/publishers/issues/1179

nvonpentz · 2018-09-17T16:34:54Z

app/models/site_banner.rb

+    elsif Rails.env.staging?
+      return "https://rewards-stg.s3.us-east-2.amazonaws.com/#{object.blob.key}"
+    elsif Rails.env.production?
+      # TODO


Should we resolve this TODO?

Yes I'll resolve. as part of this PR

nvonpentz · 2018-09-17T20:58:07Z

app/models/json_builders/channels_json_builder.rb

      verified_channels.find_each do |verified_channel|
        if @excluded_channel_ids.include?(verified_channel.details.channel_identifier)
          excluded = true
          @excluded_verified_channel_ids.push(verified_channel.details.channel_identifier)
        else
          excluded = false
        end
-        channels.push([verified_channel.details.channel_identifier, true, excluded])
+        channels.push([verified_channel.details.channel_identifier, true, excluded, verified_channel.publisher&.site_banner&.read_only_react_property || {}])


should we check to make sure this change doesn't break how the browser parses the response?

I confirmed with ryanml that this won't break anything

nvonpentz · 2018-09-17T21:36:25Z

app/models/json_builders/channels_json_builder.rb

@@ -1,5 +1,5 @@
 # Builds a list of distinct channels that are either verified, excluded or both for the Brave Browser.
-# 
+#
 # Each channel is an array:
 # [channel_identifier, verified, excluded]


We should update this comment with the new response format

Closes #1092

users

corymcdonald · 2018-09-18T15:47:11Z

app/controllers/publishers/site_banners_controller.rb

+      publisher_id: current_publisher.id,
+      title: params[:title],
+      donation_amounts: donation_amounts,
+      default_donation: donation_amounts[1],


We could use some syntactic sugar and say something like donation_amounts.second. Up to you if you wanna change it

corymcdonald · 2018-09-18T15:47:31Z

app/controllers/publishers/site_banners_controller.rb

+    donation_amounts = JSON.parse(params[:donation_amounts])
+    site_banner.update(
+      publisher_id: current_publisher.id,
+      title: params[:title],


Do we verify these params anywhere?

corymcdonald · 2018-09-18T15:51:17Z

app/models/json_builders/channels_json_builder.rb

-      Channel.youtube_channels,
-      Channel.twitch_channels,
-      Channel.twitter_channels].each do |verified_channels|
+  [Channel.left_joins(publisher: :site_banner).verified.site_channels, Channel.left_joins(publisher: :site_banner).youtube_channels, Channel.left_joins(publisher: :site_banner).twitch_channels, Channel.left_joins(publisher: :site_banner).twitter_channels].each do |verified_channels|


This is an awfully long line, and we call left_joins on this a lot. Maybe it would make sense to introduce a scope on the Channel model?

We'll fix this later.

corymcdonald · 2018-09-18T15:54:26Z

app/models/site_banner.rb

+    if Rails.env.development? || Rails.env.test?
+      "https://0.0.0.0:3000" + rails_blob_path(object, only_path: true) + extension
+    elsif Rails.env.staging?
+      return "https://rewards-stg.s3.us-east-2.amazonaws.com/#{object.blob.key}"


I'm not a huge fan of having hard coded urls in a model instead of a configuration file. What are your thoughts on the matter?

corymcdonald · 2018-09-18T15:55:06Z

app/views/publishers/_instant_donation_modal.html.slim

+                .col-md-4
+                  = link_to("Preview Banner", "#", class: 'btn btn-primary', id: "preview-banner-button")
+                .col-md-4
+                  = link_to("Customize", "#", class: 'btn btn-primary', id: "edit-banner-button")


The rest of the file is internationalized, should we do the same here?

corymcdonald · 2018-09-18T16:04:56Z

config/webpack/environment.js

@@ -1,3 +1,5 @@
 const { environment } = require('@rails/webpacker')
+const typescript =  require('./loaders/typescript')


wow! it's cool to see code I contributed to webpacker actually being used!!

* Fix spacing between buttons * Use translation strings instead of hardcoding * Remove _instant_donation_button.html.slim

Add BRAVE_REWARDS_EMAIL_WHITELIST to base.yml for docker users

…on-modal Fix instant donation modal

diracdeltas · 2018-09-18T19:26:21Z

app/controllers/publishers/site_banners_controller.rb

+
+  private
+
+  def update_image(attachment)


is there an attachment size limit anywhere? otherwise someone can try to take up all our s3 space

Addressed in f8dc6b9c53d10642367ddfddb6c199fd0b3723a6. Test is in publishers/test/controllers/publishers/site_banners_controller.rb

diracdeltas · 2018-09-18T19:32:30Z

app/javascript/packs/brave_rewards_banner_display.jsx

+
+  handleSubmit(event) {
+    const url = '/publishers/' + this.props.publisher_id + "/site_banners";
+    var request = new XMLHttpRequest();


this appears to be unused? (you can add a js lint command in travis to catch these things)

diracdeltas · 2018-09-18T19:35:46Z

package.json

+    "@types/react": "^16.4.7",
+    "@types/react-dom": "^16.0.6",
+    "babel-preset-react": "^6.24.1",
+    "brave-ui": "0.23.0",


is there any reason to pin this to a specific version?

diracdeltas · 2018-09-18T19:36:07Z

package.json

+    "react": "^16.4.1",
+    "react-dom": "^16.4.1",
+    "styled-components": "^3.2.6",
+    "ts-loader": "3.5.0",


is there any reason to pin this to a specific version?

Oh hey! I actually know the answer to this, because webpacker doesn't support webpack 4 in production, and they introduced this commit to lock it down
rails/webpacker@af471dc#diff-63cad75e7b69369d47bacd65341a3bf8

@yachtcaptain23 can you make sure that you lock this down?

diracdeltas · 2018-09-18T19:37:39Z

app/models/site_banner.rb

+    if Rails.env.development? || Rails.env.test?
+      "https://0.0.0.0:3000" + rails_blob_path(object, only_path: true) + extension
+    elsif Rails.env.staging?
+      return "https://rewards-stg.s3.us-east-2.amazonaws.com/#{object.blob.key}"


what are the permissions on this bucket?

Exposed in the URL as part of a public read only. The write permissions are stored in heroku's environment variables and configured in storage.yml

…anner

Site banner overhaul rebase

Restores modal to original size

Use uniform file size

First we calculate how much padding to add by creating a Tempfile with 1 character padding and checking the delta between the Tempfile's size and the target file size. Then we apply padding to the original file.

Feature/resize images

Fixes modal expansion bug

…size Fix file size restriction and logo image restrictions

yachtcaptain23 force-pushed the feature/brave_rewards_ui_rebase2 branch from 92c8cec to 5444f24 Compare September 11, 2018 23:04

yachtcaptain23 requested review from nvonpentz and dlipeles September 11, 2018 23:04

dlipeles approved these changes Sep 12, 2018 •

edited

Loading

View reviewed changes

nvonpentz reviewed Sep 17, 2018

View reviewed changes

yachtcaptain23 added 9 commits September 18, 2018 07:36

Migration for SiteBanner

04ba6a6

Include social links and donation amounts

4dfadcc

Brave Rewards Integration

6b20c03

Closes #1092

Remove commented code

59e7d13

Fix broken headers for editing contact info

0f69b44

Test to make sure instant donation button is only there for whitelisted

5316d07

users

Test for saving a site banner

ed1c97a

Use the latest chromedriver-helper to resolve travis install

6979ddb

Update the channels json

9dc5778

yachtcaptain23 force-pushed the feature/brave_rewards_ui_rebase2 branch from 31502bd to 9dc5778 Compare September 18, 2018 14:37

Add BRAVE_REWARDS_EMAIL_WHITELIST to base.yml for docker users

367975a

corymcdonald reviewed Sep 18, 2018

View reviewed changes

nvonpentz and others added 4 commits September 18, 2018 13:04

Fix instant donation modal

160467f

* Fix spacing between buttons * Use translation strings instead of hardcoding * Remove _instant_donation_button.html.slim

Merge pull request #7 from nvonpentz/brave-rewards/add-envar

2a503f5

Add BRAVE_REWARDS_EMAIL_WHITELIST to base.yml for docker users

Merge pull request #8 from nvonpentz/brave-rewards/fix/instant-donati…

8cdc4f6

…on-modal Fix instant donation modal

Use S3 bucket specified by ENV

6439374

diracdeltas reviewed Sep 18, 2018

View reviewed changes

yachtcaptain23 added 4 commits September 18, 2018 13:12

Use Rails.application.secrets

f583149

Don't lock the ts-loader and brave-ui packages

ca34e5c

Pending Tests: Cannot upload image

a91ea90

Disable fade as this prevents new uploads for background image

d199dc3

dlipeles and others added 28 commits September 28, 2018 20:53

Add intro, social links update, cleanup controller

a91d37e

Numerical values only for donations, fix bottom padding for rewards b…

3573733

…anner

Image cropping / Retina support for logo

a346724

Retina mobile support, automatic cropping

1a01c39

Fix image issues for local files

cf30298

Merge pull request #17 from yachtcaptain23/site-banner-overhaul-rebase

4509679

Site banner overhaul rebase

Restores modal to original size

410ff60

Continue checking against uploads of large image sizes

8561b00

Merge pull request #18 from dlipeles/feature/rewards-banner

841b573

Restores modal to original size

Include mini_magick

1cf4841

Convert to single image size

6d32826

Use uniform file size

Initial commit for site banner resizing logic

326f9eb

Fix background saving by limiting file size

6692337

Add tests for padding logo. Increased number of resizing loops

e82e3dc

Test to make sure hashing is always consistent

419028e

Improved padding calculation for image conversion.

ed19b8a

First we calculate how much padding to add by creating a Tempfile with 1 character padding and checking the delta between the Tempfile's size and the target file size. Then we apply padding to the original file.

Handle cases of file sizes being too big

07d7abf

Document file size limits for the site banner

de107af

Change resolution for publisher's logo to 240x240

bef7467

Ignore .DS_STORE file

1e20f8f

Support upscaled image size

2ca0dc6

Merge pull request #15 from yachtcaptain23/feature/resize_images

821b892

Feature/resize images

Fixes modal expansion bug

2ad4fb4

Merge pull request #19 from dlipeles/feature/rewards-banner

a1c0a28

Fixes modal expansion bug

Fix file size restriction and logo image restrictions

c52a014

Merge pull request #20 from yachtcaptain23/bugfix/brave_rewards_file_…

59c9344

…size Fix file size restriction and logo image restrictions

Fix extension to use image/jpg instead of image/jpeg

fc4ae8d

Remove debug statement

6cdd5c4

yachtcaptain23 merged commit 8e30e19 into brave-intl:staging Oct 4, 2018

yachtcaptain23 mentioned this pull request Oct 4, 2018

Revert "Feature/brave rewards ui rebase2" #1202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/brave rewards ui rebase2 #1162

Feature/brave rewards ui rebase2 #1162

yachtcaptain23 commented Sep 11, 2018

nvonpentz left a comment

nvonpentz Sep 17, 2018

yachtcaptain23 Sep 17, 2018

nvonpentz Sep 17, 2018

yachtcaptain23 Sep 17, 2018

nvonpentz Sep 17, 2018

yachtcaptain23 Sep 17, 2018

nvonpentz Sep 17, 2018

corymcdonald Sep 18, 2018

corymcdonald Sep 18, 2018

corymcdonald Sep 18, 2018

yachtcaptain23 Sep 18, 2018

corymcdonald Sep 18, 2018

corymcdonald Sep 18, 2018

corymcdonald Sep 18, 2018

diracdeltas Sep 18, 2018

yachtcaptain23 Sep 19, 2018

diracdeltas Sep 18, 2018

diracdeltas Sep 18, 2018

diracdeltas Sep 18, 2018

corymcdonald Sep 19, 2018 •

edited

Loading

diracdeltas Sep 18, 2018

yachtcaptain23 Sep 18, 2018

		@@ -1,3 +1,5 @@
		const { environment } = require('@rails/webpacker')
		const typescript = require('./loaders/typescript')

Feature/brave rewards ui rebase2 #1162

Feature/brave rewards ui rebase2 #1162

Conversation

yachtcaptain23 commented Sep 11, 2018

nvonpentz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corymcdonald Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corymcdonald Sep 19, 2018 •

edited

Loading