From fa08f027c7d80cf7f557613d3015ed9e6c6ad28e Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 18 Jul 2024 14:39:38 -0400 Subject: [PATCH] Sync notifications and troubleshooting guides (#998) * Add help articles * Broadcast sync messages as notifications * Lint fixes * more lint fixes * Remove redundant code --- Gemfile | 1 + Gemfile.lock | 2 + app/controllers/account/entries_controller.rb | 2 +- .../account/transfers_controller.rb | 2 +- app/controllers/accounts_controller.rb | 2 - app/controllers/help/articles_controller.rb | 11 +++ app/controllers/imports_controller.rb | 6 +- .../settings/hostings_controller.rb | 4 +- app/helpers/application_helper.rb | 15 +++- app/models/account.rb | 2 - app/models/account/sync.rb | 22 +++++ app/models/current.rb | 2 +- app/models/help/article.rb | 54 +++++++++++++ app/views/accounts/_alert.html.erb | 19 +++++ app/views/accounts/_sync_message.html.erb | 8 -- app/views/accounts/show.html.erb | 8 +- app/views/help/articles/show.html.erb | 7 ++ app/views/layouts/application.html.erb | 10 ++- app/views/shared/_alert.html.erb | 11 --- app/views/shared/_drawer.html.erb | 2 +- app/views/shared/_notification.html.erb | 80 +++++++++---------- config/locales/views/accounts/en.yml | 2 - config/locales/views/shared/en.yml | 2 - config/routes.rb | 4 + docs/help/placeholder.md | 10 +++ .../account/entries_controller_test.rb | 2 +- test/controllers/accounts_controller_test.rb | 2 +- .../help/articles_controller_test.rb | 18 +++++ test/controllers/imports_controller_test.rb | 6 +- .../settings/hostings_controller_test.rb | 4 +- test/fixtures/files/help_article.md | 6 ++ test/models/account/sync_test.rb | 6 ++ test/models/help/article_test.rb | 21 +++++ 33 files changed, 256 insertions(+), 97 deletions(-) create mode 100644 app/controllers/help/articles_controller.rb create mode 100644 app/models/help/article.rb create mode 100644 app/views/accounts/_alert.html.erb delete mode 100644 app/views/accounts/_sync_message.html.erb create mode 100644 app/views/help/articles/show.html.erb delete mode 100644 app/views/shared/_alert.html.erb create mode 100644 docs/help/placeholder.md create mode 100644 test/controllers/help/articles_controller_test.rb create mode 100644 test/fixtures/files/help_article.md create mode 100644 test/models/help/article_test.rb diff --git a/Gemfile b/Gemfile index 6261d8e2..52b57177 100644 --- a/Gemfile +++ b/Gemfile @@ -44,6 +44,7 @@ gem "pagy" gem "rails-settings-cached" gem "tzinfo-data", platforms: %i[ windows jruby ] gem "csv" +gem "redcarpet" group :development, :test do gem "debug", platforms: %i[ mri windows ] diff --git a/Gemfile.lock b/Gemfile.lock index ae720526..fcbcc8cc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -336,6 +336,7 @@ GEM logger rdoc (6.7.0) psych (>= 4.0.0) + redcarpet (3.6.0) regexp_parser (2.9.2) reline (0.5.9) io-console (~> 0.5) @@ -492,6 +493,7 @@ DEPENDENCIES puma (>= 5.0) rails! rails-settings-cached + redcarpet rubocop-rails-omakase ruby-lsp-rails selenium-webdriver diff --git a/app/controllers/account/entries_controller.rb b/app/controllers/account/entries_controller.rb index 21e971a1..b12ae099 100644 --- a/app/controllers/account/entries_controller.rb +++ b/app/controllers/account/entries_controller.rb @@ -31,7 +31,7 @@ class Account::EntriesController < ApplicationController else # TODO: this is not an ideal way to handle errors and should eventually be improved. # See: https://github.com/hotwired/turbo-rails/pull/367 - flash[:error] = @entry.errors.full_messages.to_sentence + flash[:alert] = @entry.errors.full_messages.to_sentence redirect_to account_path(@account) end end diff --git a/app/controllers/account/transfers_controller.rb b/app/controllers/account/transfers_controller.rb index a494285c..ac60583a 100644 --- a/app/controllers/account/transfers_controller.rb +++ b/app/controllers/account/transfers_controller.rb @@ -23,7 +23,7 @@ class Account::TransfersController < ApplicationController else # TODO: this is not an ideal way to handle errors and should eventually be improved. # See: https://github.com/hotwired/turbo-rails/pull/367 - flash[:error] = @transfer.errors.full_messages.to_sentence + flash[:alert] = @transfer.errors.full_messages.to_sentence redirect_to transactions_path end end diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 4f4c2b1e..568f440f 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -68,8 +68,6 @@ class AccountsController < ApplicationController unless @account.syncing? @account.sync_later end - - redirect_to account_path(@account), notice: t(".success") end def sync_all diff --git a/app/controllers/help/articles_controller.rb b/app/controllers/help/articles_controller.rb new file mode 100644 index 00000000..17ff8fae --- /dev/null +++ b/app/controllers/help/articles_controller.rb @@ -0,0 +1,11 @@ +class Help::ArticlesController < ApplicationController + layout "with_sidebar" + + def show + @article = Help::Article.find(params[:id]) + + unless @article + head :not_found + end + end +end diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index 0857e278..90086684 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -42,13 +42,13 @@ class ImportsController < ApplicationController begin @import.raw_csv_str = import_params[:raw_csv_str].read rescue NoMethodError - flash.now[:error] = "Please select a file to upload" + flash.now[:alert] = "Please select a file to upload" render :load, status: :unprocessable_entity and return end if @import.save redirect_to configure_import_path(@import), notice: t(".import_loaded") else - flash.now[:error] = @import.errors.full_messages.to_sentence + flash.now[:alert] = @import.errors.full_messages.to_sentence render :load, status: :unprocessable_entity end end @@ -57,7 +57,7 @@ class ImportsController < ApplicationController if @import.update(import_params) redirect_to configure_import_path(@import), notice: t(".import_loaded") else - flash.now[:error] = @import.errors.full_messages.to_sentence + flash.now[:alert] = @import.errors.full_messages.to_sentence render :load, status: :unprocessable_entity end end diff --git a/app/controllers/settings/hostings_controller.rb b/app/controllers/settings/hostings_controller.rb index 89a42946..3058074f 100644 --- a/app/controllers/settings/hostings_controller.rb +++ b/app/controllers/settings/hostings_controller.rb @@ -19,7 +19,7 @@ class Settings::HostingsController < SettingsController def send_test_email unless Setting.smtp_settings_populated? - flash[:error] = t(".missing_smtp_setting_error") + flash[:alert] = t(".missing_smtp_setting_error") render(:show, status: :unprocessable_entity) return end @@ -27,7 +27,7 @@ class Settings::HostingsController < SettingsController begin NotificationMailer.with(user: Current.user).test_email.deliver_now rescue => _e - flash[:error] = t(".error") + flash[:alert] = t(".error") render :show, status: :unprocessable_entity return end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index bb9462a5..141db192 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -13,11 +13,18 @@ module ApplicationHelper name.underscore end - def notification(text, **options, &block) - content = tag.p(text) - content = capture &block if block_given? + def family_notifications_stream + turbo_stream_from [ Current.family, :notifications ] if Current.family + end - render partial: "shared/notification", locals: { type: options[:type], content: { body: content } } + def render_flash_notifications + notifications = flash.flat_map do |type, message_or_messages| + Array(message_or_messages).map do |message| + render partial: "shared/notification", locals: { type: type, message: message } + end + end + + safe_join(notifications) end ## diff --git a/app/models/account.rb b/app/models/account.rb index 7d999835..2ce6495f 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -2,8 +2,6 @@ class Account < ApplicationRecord include Syncable include Monetizable - broadcasts_refreshes - validates :name, :balance, :currency, presence: true belongs_to :family diff --git a/app/models/account/sync.rb b/app/models/account/sync.rb index 0f8a1c9a..5db21b89 100644 --- a/app/models/account/sync.rb +++ b/app/models/account/sync.rb @@ -48,13 +48,35 @@ class Account::Sync < ApplicationRecord def start! update! status: "syncing", last_ran_at: Time.now + broadcast_start end def complete! update! status: "completed" + broadcast_result type: "notice", message: "Sync complete" end def fail!(error) update! status: "failed", error: error.message + broadcast_result type: "alert", message: error.message + end + + def broadcast_start + broadcast_append_to( + [ account.family, :notifications ], + target: "notification-tray", + partial: "shared/notification", + locals: { id: id, type: "processing", message: "Syncing account balances" } + ) + end + + def broadcast_result(type:, message:) + broadcast_remove_to account.family, :notifications, target: id # Remove persistent syncing notification + broadcast_append_to( + [ account.family, :notifications ], + target: "notification-tray", + partial: "shared/notification", + locals: { type: type, message: message } + ) end end diff --git a/app/models/current.rb b/app/models/current.rb index fd51b793..316a2cf5 100644 --- a/app/models/current.rb +++ b/app/models/current.rb @@ -1,5 +1,5 @@ class Current < ActiveSupport::CurrentAttributes attribute :user - delegate :family, to: :user + delegate :family, to: :user, allow_nil: true end diff --git a/app/models/help/article.rb b/app/models/help/article.rb new file mode 100644 index 00000000..40f79474 --- /dev/null +++ b/app/models/help/article.rb @@ -0,0 +1,54 @@ +class Help::Article + attr_reader :frontmatter, :content + + def initialize(frontmatter:, content:) + @frontmatter = frontmatter + @content = content + end + + def title + frontmatter["title"] + end + + def html + render_markdown(content) + end + + class << self + def root_path + Rails.root.join("docs", "help") + end + + def find(slug) + Dir.glob(File.join(root_path, "*.md")).each do |file_path| + file_content = File.read(file_path) + frontmatter, markdown_content = parse_frontmatter(file_content) + + return new(frontmatter:, content: markdown_content) if frontmatter["slug"] == slug + end + + nil + end + + private + + def parse_frontmatter(content) + if content =~ /\A---(.+?)---/m + frontmatter = YAML.safe_load($1) + markdown_content = content[($~.end(0))..-1].strip + else + frontmatter = {} + markdown_content = content + end + + [ frontmatter, markdown_content ] + end + end + + private + + def render_markdown(content) + markdown = Redcarpet::Markdown.new(Redcarpet::Render::HTML) + markdown.render(content) + end +end diff --git a/app/views/accounts/_alert.html.erb b/app/views/accounts/_alert.html.erb new file mode 100644 index 00000000..697f0942 --- /dev/null +++ b/app/views/accounts/_alert.html.erb @@ -0,0 +1,19 @@ +<%# locals: (message:, help_path: nil) -%> +<%= tag.div class: "flex gap-6 items-center rounded-xl px-4 py-3 bg-error/5", + data: { controller: "element-removal" }, + role: "alert" do %> +
+ <%= lucide_icon("alert-octagon", class: "w-5 h-5 shrink-0") %> +

<%= message %>

+
+ +
+ <% if help_path %> + <%= link_to "Troubleshoot", help_path, class: "text-red-500 font-medium hover:underline", data: { turbo_frame: :drawer } %> + <% end %> + + <%= tag.button data: { action: "click->element-removal#remove" } do %> + <%= lucide_icon("x", class: "w-5 h-5 shrink-0 text-red-500") %> + <% end %> +
+<% end %> diff --git a/app/views/accounts/_sync_message.html.erb b/app/views/accounts/_sync_message.html.erb deleted file mode 100644 index fe0bf405..00000000 --- a/app/views/accounts/_sync_message.html.erb +++ /dev/null @@ -1,8 +0,0 @@ -<%# locals: (is_syncing:) %> -<% if is_syncing %> -
-

- Syncing your account balances. -

-
-<% end %> diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb index de2a57e1..2459a7eb 100644 --- a/app/views/accounts/show.html.erb +++ b/app/views/accounts/show.html.erb @@ -1,5 +1,3 @@ -<%= turbo_stream_from @account %> -
@@ -45,12 +43,8 @@
- <%= turbo_frame_tag "sync_message" do %> - <%= render partial: "accounts/sync_message", locals: { is_syncing: @account.syncing? } %> - <% end %> - <% if @account.alert %> - <%= render partial: "shared/alert", locals: { type: "error", content: t("." + @account.alert) } %> + <%= render "alert", message: @account.alert, help_path: help_article_path("troubleshooting") %> <% end %>
diff --git a/app/views/help/articles/show.html.erb b/app/views/help/articles/show.html.erb new file mode 100644 index 00000000..d2a9eeff --- /dev/null +++ b/app/views/help/articles/show.html.erb @@ -0,0 +1,7 @@ +<%= drawer do %> +
+ <%= tag.h1 @article.title %> + + <%= sanitize(@article.html).html_safe %> +
+<% end %> diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 6fe949fb..ab57b5d3 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -25,8 +25,13 @@ -
- <%= safe_join(flash.map { |type, message| notification(message, type: type) }) %> +
+
+ <%= render_flash_notifications %> +
+
+ + <%= family_notifications_stream %> <%= content_for?(:content) ? yield(:content) : yield %> @@ -39,5 +44,4 @@ <%= render "shared/app_version" %> <% end %> - diff --git a/app/views/shared/_alert.html.erb b/app/views/shared/_alert.html.erb deleted file mode 100644 index 21091fe4..00000000 --- a/app/views/shared/_alert.html.erb +++ /dev/null @@ -1,11 +0,0 @@ -<%# locals: (type: "error", content: "") -%> -<%= content_tag :div, - class: "flex justify-between rounded-xl p-3 #{type == "error" ? "bg-red-50" : "bg-yellow-50"}", - data: {controller: "element-removal" }, - role: type == "error" ? "alert" : "status" do %> -
"> - <%= lucide_icon("info", class: "w-5 h-5 shrink-0") %> -

<%= content %>

-
- <%= content_tag :a, lucide_icon("x", class: "w-5 h-5 shrink-0 #{type == "error" ? "text-red-500" : "text-yellow-500"}"), data: { action: "click->element-removal#remove" }, class:"flex gap-1 font-medium items-center text-gray-900 px-3 py-1.5 rounded-lg cursor-pointer" %> -<% end %> diff --git a/app/views/shared/_drawer.html.erb b/app/views/shared/_drawer.html.erb index 321667c8..4bce63fc 100644 --- a/app/views/shared/_drawer.html.erb +++ b/app/views/shared/_drawer.html.erb @@ -1,5 +1,5 @@ <%= turbo_frame_tag "drawer" do %> - +
diff --git a/app/views/shared/_notification.html.erb b/app/views/shared/_notification.html.erb index 9e3a1706..48e4c731 100644 --- a/app/views/shared/_notification.html.erb +++ b/app/views/shared/_notification.html.erb @@ -1,45 +1,45 @@ -<%# locals: (type: "success", content: { title: '', body: ''}, action: { label:'' , url:'' }, options: { auto_dismiss: true }) -%> +<%# locals: (message:, type: "notice", id: nil, **_opts) %> - - - +<% end %> diff --git a/config/locales/views/accounts/en.yml b/config/locales/views/accounts/en.yml index f838f961..b9f425fd 100644 --- a/config/locales/views/accounts/en.yml +++ b/config/locales/views/accounts/en.yml @@ -66,8 +66,6 @@ en: value: Value summary: new: New account - sync: - success: Account sync started sync_all: success: Successfully queued accounts for syncing. update: diff --git a/config/locales/views/shared/en.yml b/config/locales/views/shared/en.yml index 5df96bb3..46ad461e 100644 --- a/config/locales/views/shared/en.yml +++ b/config/locales/views/shared/en.yml @@ -13,8 +13,6 @@ en: no_account_subtitle: Since no accounts have been added, there's no data to display. Add your first accounts to start viewing dashboard data. no_account_title: No accounts yet - notification: - dismiss: Dismiss upgrade_notification: app_upgraded: The app has been upgraded to %{version}. dismiss: Dismiss diff --git a/config/routes.rb b/config/routes.rb index ea5d31d5..093ac809 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -10,6 +10,10 @@ Rails.application.routes.draw do resource :password_reset resource :password + namespace :help do + resources :articles, only: :show + end + namespace :settings do resource :profile, only: %i[show update destroy] resource :preferences, only: %i[show update] diff --git a/docs/help/placeholder.md b/docs/help/placeholder.md new file mode 100644 index 00000000..3ede4e8b --- /dev/null +++ b/docs/help/placeholder.md @@ -0,0 +1,10 @@ +--- +title: Troubleshooting +slug: troubleshooting +--- + +Coming soon... + +We're working on new guides to help troubleshoot various issues within the app. + +Help us out by reporting [issues on Github](https://github.com/maybe-finance/maybe/issues). diff --git a/test/controllers/account/entries_controller_test.rb b/test/controllers/account/entries_controller_test.rb index 48e2c1c1..5442fae5 100644 --- a/test/controllers/account/entries_controller_test.rb +++ b/test/controllers/account/entries_controller_test.rb @@ -69,7 +69,7 @@ class Account::EntriesControllerTest < ActionDispatch::IntegrationTest } end - assert_equal "Date has already been taken", flash[:error] + assert_equal "Date has already been taken", flash[:alert] assert_redirected_to account_path(@valuation.account) end diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index dfed6d36..09e64c09 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -27,7 +27,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest test "can sync an account" do post sync_account_path(@account) - assert_redirected_to account_url(@account) + assert_response :no_content end test "can sync all accounts" do diff --git a/test/controllers/help/articles_controller_test.rb b/test/controllers/help/articles_controller_test.rb new file mode 100644 index 00000000..42f4998d --- /dev/null +++ b/test/controllers/help/articles_controller_test.rb @@ -0,0 +1,18 @@ +require "test_helper" + +class Help::ArticlesControllerTest < ActionDispatch::IntegrationTest + setup do + sign_in @user = users(:family_admin) + + @article = Help::Article.new(frontmatter: { title: "Test Article", slug: "test-article" }, content: "") + + Help::Article.stubs(:find).returns(@article) + end + + test "can view help article" do + get help_article_path(@article) + + assert_response :success + assert_dom "h1", text: @article.title, count: 1 + end +end diff --git a/test/controllers/imports_controller_test.rb b/test/controllers/imports_controller_test.rb index e7cdbc3c..b1798c77 100644 --- a/test/controllers/imports_controller_test.rb +++ b/test/controllers/imports_controller_test.rb @@ -81,7 +81,7 @@ class ImportsControllerTest < ActionDispatch::IntegrationTest patch load_import_url(@empty_import), params: { import: { raw_csv_str: malformed_csv_str } } assert_response :unprocessable_entity - assert_equal "Raw csv str is not a valid CSV format", flash[:error] + assert_equal "Raw csv str is not a valid CSV format", flash[:alert] end test "should flash error message if invalid CSV file upload" do @@ -91,14 +91,14 @@ class ImportsControllerTest < ActionDispatch::IntegrationTest patch upload_import_url(@empty_import), params: { import: { raw_csv_str: Rack::Test::UploadedFile.new(temp, ".csv") } } assert_response :unprocessable_entity - assert_equal "Raw csv str is not a valid CSV format", flash[:error] + assert_equal "Raw csv str is not a valid CSV format", flash[:alert] end end test "should flash error message if no fileprovided for upload" do patch upload_import_url(@empty_import), params: { import: { raw_csv_str: nil } } assert_response :unprocessable_entity - assert_equal "Please select a file to upload", flash[:error] + assert_equal "Please select a file to upload", flash[:alert] end test "should get configure" do diff --git a/test/controllers/settings/hostings_controller_test.rb b/test/controllers/settings/hostings_controller_test.rb index 81db36ff..05e70789 100644 --- a/test/controllers/settings/hostings_controller_test.rb +++ b/test/controllers/settings/hostings_controller_test.rb @@ -76,7 +76,7 @@ class Settings::HostingsControllerTest < ActionDispatch::IntegrationTest post send_test_email_settings_hosting_path assert_response :unprocessable_entity - assert controller.flash[:error].present? + assert controller.flash[:alert].present? end end @@ -87,7 +87,7 @@ class Settings::HostingsControllerTest < ActionDispatch::IntegrationTest post send_test_email_settings_hosting_path assert_response :unprocessable_entity - assert controller.flash[:error].present? + assert controller.flash[:alert].present? end end end diff --git a/test/fixtures/files/help_article.md b/test/fixtures/files/help_article.md new file mode 100644 index 00000000..b2dc0b3d --- /dev/null +++ b/test/fixtures/files/help_article.md @@ -0,0 +1,6 @@ +--- +title: Placeholder +slug: placeholder +--- + +Test help article \ No newline at end of file diff --git a/test/models/account/sync_test.rb b/test/models/account/sync_test.rb index 564889ce..1167a41d 100644 --- a/test/models/account/sync_test.rb +++ b/test/models/account/sync_test.rb @@ -26,9 +26,15 @@ class Account::SyncTest < ActiveSupport::TestCase @sync.run + streams = capture_turbo_stream_broadcasts [ @account.family, :notifications ] + assert_equal "completed", @sync.status assert_equal [ "test balance sync warning", "test holding sync warning" ], @sync.warnings assert @sync.last_ran_at + + assert_equal "append", streams.first["action"] + assert_equal "remove", streams.second["action"] + assert_equal "append", streams.third["action"] end test "handles sync errors" do diff --git a/test/models/help/article_test.rb b/test/models/help/article_test.rb new file mode 100644 index 00000000..eaac5377 --- /dev/null +++ b/test/models/help/article_test.rb @@ -0,0 +1,21 @@ +require "test_helper" + +class Help::ArticleTest < ActiveSupport::TestCase + include ActiveJob::TestHelper + + setup do + Help::Article.stubs(:root_path).returns(Rails.root.join("test", "fixtures", "files")) + end + + test "returns nil if article not found" do + assert_nil Help::Article.find("missing") + end + + test "find and renders markdown article" do + article = Help::Article.find("placeholder") + + assert_equal "Placeholder", article.title + assert_equal "Test help article", article.content + assert_equal "

Test help article

\n", article.html + end +end