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

AO3-5323 Disable support form #3398

Merged
merged 7 commits into from
Aug 22, 2018

Conversation

ariana-paris
Copy link
Contributor

Issue

https://otwarchive.atlassian.net/browse/AO3-5323

Purpose

Adds a checkbox and corresponding textbox to the Admin settings so the support form can be replaced by a message.

Testing

Tick the box and add text to the field, and check that /support displays that text instead of the support form.

@@ -0,0 +1,7 @@
class AddDisableSupportFormToAdminSettings < ActiveRecord::Migration[5.1]

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

Copy link
Member

Choose a reason for hiding this comment

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

There's a sliiiight chance the timestamp on this might give us some issues because we've run newer migrations. If it does, though, we know where to find you. 😉

Copy link
Contributor

@elzj elzj left a comment

Choose a reason for hiding this comment

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

At some point, we need to clean up all the javascript and centralize and organize it, but that day is not today

@@ -4,6 +4,7 @@ class FeedbacksController < ApplicationController

def new
@feedback = Feedback.new
@admin_setting = AdminSetting.first || AdminSetting.create(last_updated_by: Admin.first)
Copy link
Member

Choose a reason for hiding this comment

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

Following #3382 should this be AdminSetting.current?

@@ -79,3 +94,15 @@

<p><%= ts("Last updated on %{updated_at} by %{admin_name}.", :updated_at => @admin_setting.updated_at, :admin_name => @admin_setting.last_updated ? @admin_setting.last_updated.login : "---") %></p>
<!--/content-->


<%= content_for :footer_js do %>
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this isn't actually toggling the form field (I don't see the toggle_formfield class), so I think this can be removed. If we are actually toggling the field, though, this should really use the newer toggleFormField(element_id) function from application.js here, since as Elz pointed out, we are trying to prefer our centralized JavaScript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. I removed the toggle because I couldn't get the check box to both update the parameter and toggle the visibility of the textbox, so I gave up and forgot to remove the Javascript 🤦‍♀️ - I'll try to get to this later in the week! (pls poke me in Slack if no visible action by the weekend)

@sarken
Copy link
Member

sarken commented Aug 9, 2018

Just marking Needs Coder Action because there are some fresh text changes we want in this pull request.

<% else %>
<h3 class="heading"><%= ts("Please use this form for questions about how to use the Archive and to report any technical problems. We are always glad to hear from our users!") %></h3>
<div class="userstuff">
<p><%= ts("If your question or problem concerns potential violations of the Terms of Service, control of Orphaned Works, or issues requiring identity verification such as regaining access to a lost account, please <a href=\"abuse_reports/new\">contact our Policy and Abuse team</a> instead.").html_safe %></p>
Copy link
Member

Choose a reason for hiding this comment

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

To minimize the HTML Translation will need to work with, we've been moving toward doing links with interpolation instead:

<%= ts("If your question or problem concerns potential violations of the Terms of Service, control of Orphaned Works, or issues requiring identity verification such as regaining access to a lost account, please %{contact_abuse_link} instead.", contact_abuse_link: link_to(ts("contact our Policy and Abuse team"), new_abuse_report_path)).html_safe %>

I was going to merge it anyway since it's not a huge deal, but I see it has merge conflicts still. Could you update this and the link in the next paragraph when you resolve those? Then this should be good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I was in a rush to grab a small window of opportunity so went with the simple link, but this is much better. Updated and also changed the date on the migration.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yay, thanks for updating the migration as well! It looks like the Twitter link in the next paragraph didn't get updated to the new format, but since LO is trying to wrap up testing pre-hurricane, I'm going to merge this anyway.

@sarken sarken added Reviewed: Action Needed Has Migrations Contains migrations and therefore needs special attention when deploying Has Production Config Changes Modifies the config file and needs special attention when deploying and removed Coder Has Actioned Review labels Aug 22, 2018
@@ -0,0 +1,7 @@
class AddDisableSupportFormToAdminSettings < ActiveRecord::Migration[5.1]

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@sarken sarken merged commit de401af into otwcode:master Aug 22, 2018
@ariana-paris ariana-paris deleted the AO3-5323-disable-support-form branch August 22, 2018 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Migrations Contains migrations and therefore needs special attention when deploying Has Production Config Changes Modifies the config file and needs special attention when deploying Reviewed: Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants