From f34b2203d304451a874fe7b8bb5b5996122ac9b9 Mon Sep 17 00:00:00 2001 From: Fernando Valverde Date: Thu, 24 Jun 2021 08:36:11 -0600 Subject: [PATCH] Make Consumer Apps dictate aasa results (#14015) * Makes Consumer Apps dictate aasa results * progress with ConsumerApp query * Adds Team ID migration + Stimulus consumer_app_controller.js * Adds cypress tests * Adds Backfill data_update_script + more specs & tweaks * Remove file added by mistake * Comment typo * Small tweaks + improved specs * Update lib/data_update_scripts/20210622145212_backfill_forem_consumer_app_team_id.rb Co-authored-by: Jamie Gaskins * Update spec/lib/data_update_scripts/backfill_forem_consumer_app_team_id_spec.rb Co-authored-by: rhymes * Make use of create! and log errors to ForemStatsClient * Fix specs * Add mock_rpush call as suggested in review * Add Review suggestions * Fix tests * Remove redundant assert in spec Co-authored-by: Jamie Gaskins Co-authored-by: rhymes --- app/controllers/deep_links_controller.rb | 16 +++++-- .../controllers/consumer_app_controller.js | 17 +++++++ app/models/consumer_app.rb | 1 + .../consumer_apps/find_or_create_all_query.rb | 11 ++++- .../consumer_apps/find_or_create_by_query.rb | 4 +- app/views/admin/consumer_apps/_form.html.erb | 7 ++- app/views/admin/consumer_apps/edit.html.erb | 2 +- app/views/admin/consumer_apps/new.html.erb | 2 +- .../consumerApps/createConsumerApp.spec.js | 46 +++++++++++++++++++ ...0622002941_add_team_id_to_consumer_apps.rb | 5 ++ db/schema.rb | 3 +- ...212_backfill_forem_consumer_app_team_id.rb | 9 ++++ ...ackfill_forem_consumer_app_team_id_spec.rb | 34 ++++++++++++++ .../find_or_create_all_query_spec.rb | 3 ++ .../find_or_create_by_query_spec.rb | 1 + .../consumer_apps/rpush_app_query_spec.rb | 5 +- spec/requests/universal_links_spec.rb | 15 +++--- 17 files changed, 161 insertions(+), 20 deletions(-) create mode 100644 app/javascript/admin/controllers/consumer_app_controller.js create mode 100644 cypress/integration/adminFlows/consumerApps/createConsumerApp.spec.js create mode 100644 db/migrate/20210622002941_add_team_id_to_consumer_apps.rb create mode 100644 lib/data_update_scripts/20210622145212_backfill_forem_consumer_app_team_id.rb create mode 100644 spec/lib/data_update_scripts/backfill_forem_consumer_app_team_id_spec.rb diff --git a/app/controllers/deep_links_controller.rb b/app/controllers/deep_links_controller.rb index 4df329b99..326770e4c 100644 --- a/app/controllers/deep_links_controller.rb +++ b/app/controllers/deep_links_controller.rb @@ -4,10 +4,18 @@ class DeepLinksController < ApplicationController # Apple Application Site Association - based on Apple docs guidelines # https://developer.apple.com/library/archive/documentation/General/Conceptual/AppSearch/UniversalLinks.html def aasa - # TODO: [@fdoxyz] Replace these hardcoded identifiers with configurations - # creators can use to customize their Forems - `/admin/consumer_apps` - supported_apps = ["R9SWHSQNV8.com.forem.app"] - supported_apps << "R9SWHSQNV8.to.dev.ios" if ForemInstance.dev_to? + # This query plucks :team_id & :app_bundle so we get an array or arrays + # Example: [['TEAM1', 'app.bundle.one'], ['TEAM2', 'app.bundle.two']] + consumer_apps = ConsumerApps::FindOrCreateAllQuery.call + .where(platform: Device::IOS) + .where.not(team_id: nil) + .order(:created_at) + .pluck(:team_id, :app_bundle) + + # Now restructure the array of arrays into valid AASA App ID's + # Example: ['TEAM1.app.bundle.one', 'TEAM2.app.bundle.two'] + supported_apps = consumer_apps.map { |result| result.join(".") } + render json: { applinks: { apps: [], diff --git a/app/javascript/admin/controllers/consumer_app_controller.js b/app/javascript/admin/controllers/consumer_app_controller.js new file mode 100644 index 000000000..0a39a5699 --- /dev/null +++ b/app/javascript/admin/controllers/consumer_app_controller.js @@ -0,0 +1,17 @@ +import { Controller } from 'stimulus'; + +export default class ConsumerAppController extends Controller { + static targets = ['platform', 'teamId']; + + connect() { + this.checkPlatform(); + } + + checkPlatform() { + if (this.platformTarget.value === 'ios') { + this.teamIdTarget.classList.remove('hidden'); + } else { + this.teamIdTarget.classList.add('hidden'); + } + } +} diff --git a/app/models/consumer_app.rb b/app/models/consumer_app.rb index 4cb10ba36..39e1a463c 100644 --- a/app/models/consumer_app.rb +++ b/app/models/consumer_app.rb @@ -3,6 +3,7 @@ class ConsumerApp < ApplicationRecord FOREM_BUNDLE = "com.forem.app".freeze FOREM_APP_PLATFORMS = %w[ios].freeze + FOREM_TEAM_ID = "R9SWHSQNV8".freeze enum platform: { android: Device::ANDROID, ios: Device::IOS } diff --git a/app/queries/consumer_apps/find_or_create_all_query.rb b/app/queries/consumer_apps/find_or_create_all_query.rb index 3593dda8f..e2ff55efb 100644 --- a/app/queries/consumer_apps/find_or_create_all_query.rb +++ b/app/queries/consumer_apps/find_or_create_all_query.rb @@ -5,7 +5,16 @@ module ConsumerApps existing_platforms = ConsumerApp.where(app_bundle: forem_bundle).pluck(:platform) (ConsumerApp::FOREM_APP_PLATFORMS - existing_platforms).each do |platform| # Re-create the supported Forem apps if they're missing - ConsumerApp.create(app_bundle: forem_bundle, platform: platform, active: true) + ConsumerApp.create!(app_bundle: forem_bundle, + platform: platform, + team_id: ConsumerApp::FOREM_TEAM_ID) + rescue StandardError => e + error_tags = [ + "error:#{e.message}", + "app_bundle:#{forem_bundle}", + "platform:#{platform}", + ] + ForemStatsClient.increment("consumer_apps.create", tags: error_tags) end ConsumerApp.limit(50) diff --git a/app/queries/consumer_apps/find_or_create_by_query.rb b/app/queries/consumer_apps/find_or_create_by_query.rb index 6e99a2a85..ae9bcb955 100644 --- a/app/queries/consumer_apps/find_or_create_by_query.rb +++ b/app/queries/consumer_apps/find_or_create_by_query.rb @@ -28,7 +28,9 @@ module ConsumerApps end def forem_consumer_app - relation.create_or_find_by(app_bundle: ConsumerApp::FOREM_BUNDLE) + # iOS Forem Consumer App requires a team_id value but Android (future) doesn't + team_id = platform == :ios ? ConsumerApp::FOREM_TEAM_ID : nil + relation.create_or_find_by(app_bundle: ConsumerApp::FOREM_BUNDLE, team_id: team_id) end end end diff --git a/app/views/admin/consumer_apps/_form.html.erb b/app/views/admin/consumer_apps/_form.html.erb index 7975be2ff..e20f05401 100644 --- a/app/views/admin/consumer_apps/_form.html.erb +++ b/app/views/admin/consumer_apps/_form.html.erb @@ -5,10 +5,15 @@
<%= label_tag :platform, "Platform:" %> - <%= select_tag :platform, options_for_select(ConsumerApp.platforms.invert, selected: @app.platform, class: "crayons-select"), class: "crayons-select" %> + <%= select_tag :platform, options_for_select(ConsumerApp.platforms.invert, selected: @app.platform, class: "crayons-select"), class: "crayons-select", data: { "consumer-app-target" => "platform", "action" => "consumer-app#checkPlatform" } %>
<%= label_tag :auth_key, "Authentication Key:" %> <%= text_area_tag :auth_key, @app.auth_key, size: "100x10", class: "form-control" %>
+ + diff --git a/app/views/admin/consumer_apps/edit.html.erb b/app/views/admin/consumer_apps/edit.html.erb index d04f36d00..a230d1d83 100644 --- a/app/views/admin/consumer_apps/edit.html.erb +++ b/app/views/admin/consumer_apps/edit.html.erb @@ -1,6 +1,6 @@

Edit Consumer App

- <%= form_for([:admin, @app], method: :patch) do %> + <%= form_for([:admin, @app], method: :patch, data: { controller: "consumer-app" }) do %> <%= render "form" %> <%= submit_tag "Update Consumer App", class: "crayons-btn" %> <% end %> diff --git a/app/views/admin/consumer_apps/new.html.erb b/app/views/admin/consumer_apps/new.html.erb index 41028b9e8..f37999be2 100644 --- a/app/views/admin/consumer_apps/new.html.erb +++ b/app/views/admin/consumer_apps/new.html.erb @@ -1,6 +1,6 @@

New Consumer App

- <%= form_for([:admin, @app], method: :post) do %> + <%= form_for([:admin, @app], method: :post, data: { controller: "consumer-app" }) do %> <%= render "form" %> <%= submit_tag "Create Consumer App", class: "crayons-btn" %> <% end %> diff --git a/cypress/integration/adminFlows/consumerApps/createConsumerApp.spec.js b/cypress/integration/adminFlows/consumerApps/createConsumerApp.spec.js new file mode 100644 index 000000000..f51f48074 --- /dev/null +++ b/cypress/integration/adminFlows/consumerApps/createConsumerApp.spec.js @@ -0,0 +1,46 @@ +describe('Consumer Apps', () => { + beforeEach(() => { + cy.testSetup(); + cy.fixture('users/adminUser.json').as('user'); + + cy.get('@user').then((user) => { + cy.loginUser(user); + cy.visit('/admin/apps/consumer_apps'); + }); + }); + + it('creates a new iOS Consumer App', () => { + const test_app_bundle = 'com.app.bundle'; + cy.get('.crayons-btn').contains('New Consumer App').click(); + cy.get('#new_consumer_app').as('consumerAppForm'); + cy.get('@consumerAppForm').find('#app_bundle').type(test_app_bundle); + cy.get('@consumerAppForm').find('#platform').select('iOS'); + + // iOS apps need to provide the option to add a Team ID value + cy.get('@consumerAppForm').find('#team_id').should('be.visible'); + cy.get('@consumerAppForm').find('#team_id').type('ABC123'); + cy.get('@consumerAppForm') + .get('.crayons-btn') + .contains('Create Consumer App') + .click(); + + cy.findByText(`${test_app_bundle} has been created!`).should('be.visible'); + }); + + it('creates a new Android Consumer App', () => { + const test_app_bundle = 'com.app.bundle'; + cy.get('.crayons-btn').contains('New Consumer App').click(); + cy.get('#new_consumer_app').as('consumerAppForm'); + cy.get('@consumerAppForm').find('#app_bundle').type(test_app_bundle); + cy.get('@consumerAppForm').find('#platform').select('Android'); + + // Android apps don't need a Team ID value + cy.get('@consumerAppForm').find('#team_id').should('not.be.visible'); + cy.get('@consumerAppForm') + .get('.crayons-btn') + .contains('Create Consumer App') + .click(); + + cy.findByText(`${test_app_bundle} has been created!`).should('be.visible'); + }); +}); diff --git a/db/migrate/20210622002941_add_team_id_to_consumer_apps.rb b/db/migrate/20210622002941_add_team_id_to_consumer_apps.rb new file mode 100644 index 000000000..0c2a85ce9 --- /dev/null +++ b/db/migrate/20210622002941_add_team_id_to_consumer_apps.rb @@ -0,0 +1,5 @@ +class AddTeamIdToConsumerApps < ActiveRecord::Migration[6.1] + def change + add_column :consumer_apps, :team_id, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 4d64f3364..cc0eabb9a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_06_09_164958) do +ActiveRecord::Schema.define(version: 2021_06_22_002941) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -409,6 +409,7 @@ ActiveRecord::Schema.define(version: 2021_06_09_164958) do t.datetime "created_at", precision: 6, null: false t.string "last_error" t.string "platform", null: false + t.string "team_id" t.datetime "updated_at", precision: 6, null: false t.index ["app_bundle", "platform"], name: "index_consumer_apps_on_app_bundle_and_platform", unique: true end diff --git a/lib/data_update_scripts/20210622145212_backfill_forem_consumer_app_team_id.rb b/lib/data_update_scripts/20210622145212_backfill_forem_consumer_app_team_id.rb new file mode 100644 index 000000000..c2e3afad7 --- /dev/null +++ b/lib/data_update_scripts/20210622145212_backfill_forem_consumer_app_team_id.rb @@ -0,0 +1,9 @@ +module DataUpdateScripts + class BackfillForemConsumerAppTeamId + def run + ConsumerApp + .where(app_bundle: ConsumerApp::FOREM_BUNDLE, platform: :ios) + .update(team_id: ConsumerApp::FOREM_TEAM_ID) + end + end +end diff --git a/spec/lib/data_update_scripts/backfill_forem_consumer_app_team_id_spec.rb b/spec/lib/data_update_scripts/backfill_forem_consumer_app_team_id_spec.rb new file mode 100644 index 000000000..941331922 --- /dev/null +++ b/spec/lib/data_update_scripts/backfill_forem_consumer_app_team_id_spec.rb @@ -0,0 +1,34 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20210622145212_backfill_forem_consumer_app_team_id.rb", +) + +describe DataUpdateScripts::BackfillForemConsumerAppTeamId do + def forem_ios_consumer_app + ConsumerApp.find_or_create_by(app_bundle: ConsumerApp::FOREM_BUNDLE, platform: :ios) + end + + before do + consumer_app = forem_ios_consumer_app + consumer_app.update_columns(team_id: nil) + mock_rpush(consumer_app) + end + + it "adds the team_id to a the Forem iOS Consumer App" do + # The team_id is expected be nil at the start of the test + consumer_app = forem_ios_consumer_app + expect(consumer_app.team_id).to be_nil + + expect { described_class.new.run } + .to change { consumer_app.reload.team_id } + .from(nil).to(ConsumerApp::FOREM_TEAM_ID) + end + + it "doesn't affect other Consumer App Team IDs", :aggregate_failures do + custom_team_id = "ABC123" + custom_consumer_app = create(:consumer_app, team_id: custom_team_id) + + expect { described_class.new.run } + .not_to change { custom_consumer_app.reload.team_id } + end +end diff --git a/spec/queries/consumer_apps/find_or_create_all_query_spec.rb b/spec/queries/consumer_apps/find_or_create_all_query_spec.rb index 52d3f8164..5a73c0d86 100644 --- a/spec/queries/consumer_apps/find_or_create_all_query_spec.rb +++ b/spec/queries/consumer_apps/find_or_create_all_query_spec.rb @@ -6,10 +6,13 @@ RSpec.describe ConsumerApps::FindOrCreateAllQuery, type: :query do it "fetches all ConsumerApp including the Forem apps" do all_consumer_apps = described_class.call result_bundles = all_consumer_apps.map(&:app_bundle).uniq + result_team_ids = all_consumer_apps.map(&:team_id).uniq # Must return consumer_app + every ConsumerApp::FOREM_APP_PLATFORMS expect(all_consumer_apps.count).to eq(1 + ConsumerApp::FOREM_APP_PLATFORMS.count) # Must include the consumer_app and Forem App bundles expect(result_bundles).to include(consumer_app.app_bundle, ConsumerApp::FOREM_BUNDLE) + # Must include the Forem Team ID + expect(result_team_ids).to include(ConsumerApp::FOREM_TEAM_ID) end end diff --git a/spec/queries/consumer_apps/find_or_create_by_query_spec.rb b/spec/queries/consumer_apps/find_or_create_by_query_spec.rb index 04626b089..da5cd0c96 100644 --- a/spec/queries/consumer_apps/find_or_create_by_query_spec.rb +++ b/spec/queries/consumer_apps/find_or_create_by_query_spec.rb @@ -11,6 +11,7 @@ RSpec.describe ConsumerApps::FindOrCreateByQuery, type: :query do platform: :ios, ) expect(app).to be_instance_of(ConsumerApp) + expect(app.team_id).to eq(ConsumerApp::FOREM_TEAM_ID) end.to change(ConsumerApp, :count).by(1) end end diff --git a/spec/queries/consumer_apps/rpush_app_query_spec.rb b/spec/queries/consumer_apps/rpush_app_query_spec.rb index e9c38baee..e3f86e10f 100644 --- a/spec/queries/consumer_apps/rpush_app_query_spec.rb +++ b/spec/queries/consumer_apps/rpush_app_query_spec.rb @@ -10,10 +10,7 @@ RSpec.describe ConsumerApps::RpushAppQuery, type: :query do mock_rpush(consumer_app) # Fetch rpush app associated to the target - rpush_app = described_class.call( - app_bundle: consumer_app.app_bundle, - platform: consumer_app.platform, - ) + rpush_app = described_class.call(app_bundle: consumer_app.app_bundle, platform: :ios) expect(rpush_app).to be_instance_of(Rpush::Apns2::App) expect(rpush_app.name).to eq(consumer_app.app_bundle) diff --git a/spec/requests/universal_links_spec.rb b/spec/requests/universal_links_spec.rb index ba6d67d24..903ee0ca6 100644 --- a/spec/requests/universal_links_spec.rb +++ b/spec/requests/universal_links_spec.rb @@ -3,16 +3,19 @@ require "rails_helper" RSpec.describe "Universal Links (Apple)", type: :request do let(:aasa_route) { "/.well-known/apple-app-site-association" } let(:forem_app_id) { "R9SWHSQNV8.com.forem.app" } - let(:dev_app_id) { "R9SWHSQNV8.to.dev.ios" } describe "returns a valid Apple App Site Association file" do - context "with DEV app backwards compatibility" do - it "responds with applinks support for both" do - allow(ForemInstance).to receive(:dev_to?).and_return(true) + context "with multiple ConsumerApps" do + it "responds with applinks support for iOS apps only" do + # This iOS ConsumerApp should appear in the results + ios_app = create(:consumer_app, platform: Device::IOS) + # This Android ConsumerApp shouldn't appear in the results + create(:consumer_app, platform: Device::ANDROID) + get aasa_route json_response = JSON.parse(response.body) - both_app_ids = [forem_app_id, dev_app_id] + both_app_ids = [forem_app_id, ios_app.app_bundle] expect(response).to have_http_status(:ok) expect(json_response.dig("applinks", "apps")).to be_empty json_response.dig("applinks", "details").each do |hash| @@ -22,7 +25,7 @@ RSpec.describe "Universal Links (Apple)", type: :request do end end - context "when non-DEV Forem instance" do + context "without any custom ConsumerApps" do it "responds with applinks support for Forem app" do get aasa_route json_response = JSON.parse(response.body)