Save Confirmation - Admin (Part 1 of 3) #14418

msarit · 2021-08-04T17:08:20Z

What type of PR is this? (check all applicable)

Description

Part 1 of 2 implementing the Admin-Save-Confirmation RFC.
In this PR, I refactor all confirmation steps in the /admin/customization/config page to use async calls. I also update all associated requests and end-to-end tests.

The RFC: https://github.com/forem/rfcs/pull/78/

NOTES

Do not let the number of Files changed alarm you 😅 For about 10 files, I merely added the Update Settings button, and added the updateConfigurationSettings callback to the form therein.
It has been observed that there is no visual indication if an update failed or didn't "take". I have noted this and will address this UX-improvement in a future PR. 👍🏾

QA Instructions, Screenshots, Recordings

Login to http://localhost:xxxx/admin/customization/config as a super_admin
Make various changes to the settings on this page. After clicking the Update Settings button each time, there should be NO page refresh, and a snackbar with message Successfully updated settings should appear in the bottom-left corner.
Run spec/requests/admin/configs_spec.rb and all tests under cypress/integration/seededFlows/adminFlows/config and make sure they pass.

UI accessibility concerns?

None. No major UI changes made; most changes are backend

Added/updated tests?

Yes
No, and this is why: please replace this line with details on why tests
have not been included
I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

I've updated the Developer Docs and/or
Admin Guide, or
Storybook (for Crayons components)
I've updated the README or added inline documentation
I've added an entry to
CHANGELOG.md
I will share this change in a Changelog
or in a forem.dev post
I will share this change internally with the appropriate teams
I will update the Admin Guide
I'm not sure how best to communicate this change and need help
This change does not need to be communicated, and this is why not: please
replace this line with details on why this change doesn't need to be
shared

What gif best describes this PR or how it makes you feel?

citizen428 · 2021-08-23T04:31:53Z

spec/requests/admin/configs_spec.rb

@@ -807,15 +807,17 @@
        end

        it "unsets appropriate SMTP config, and apply default value if applicable" do
-          Settings::SMTP.address = "smtp.example.com"
-          Settings::SMTP.port = 12_345
+          default_address = ApplicationConfig["SMTP_ADDRESS"].presence || nil


We do have Settings::SMTP.address and Settings::SMTP.port which have default values set through the environment, why are we doing this manually here?

field :address, type: :string, default: ApplicationConfig["SMTP_ADDRESS"].presence ... field :port, type: :integer, default: ApplicationConfig["SMTP_PORT"].presence || 25

Hey @citizen428, locally and on Travis, this spec kept failing, specifically the expectation that the default port was 25 (locally it was 2525 for me). I thought it more robust to expect whatever the default port was on the system running the spec. Same for the default address.

😄 I understand and agree with the reasoning. I was more wondering why we use ApplicationConfig directly instead of

Settings::SMTP.get_field(:smtp_port)[:default]

Which is a bit more general.

Hey @citizen428,

So locally, this is what I get for each of these Settings values:

Settings::SMTP.get_field(:smtp_address)[:default] => nil Settings::SMTP.get_field(:smtp_port)[:default] => nil ApplicationConfig["SMTP_PORT"] => "2525" ApplicationConfig["SMTP_ADDRESS"] => "smtp.mailtrap.io"

The expectations in the relevant configs_spec.rb specs are now failing when I use the more general Settings::SMTP.get_field.

I'm not sure why these values differ from each other. I think I'll keep use of ApplicationConfig; thoughts?

It's fine to use ApplicationConfig of course 😃

spec/requests/admin/configs_spec.rb

aitchiss

Took this for a spin again locally and looks good to me - nice one @msarit ✨

aitchiss · 2021-08-24T10:51:45Z

cypress/integration/seededFlows/adminFlows/config/authenticationSection.spec.js

+            cy.findByRole('alert').should(
+              'have.text',
+              'Successfully updated settings.',
+            );


aitchiss · 2021-08-24T10:54:15Z

cypress/integration/seededFlows/adminFlows/config/authenticationSection.spec.js

+        cy.get('#facebook-auth-btn').click();
+        cy.get('#settings_authentication_facebook_key').type('randomkey');
+        cy.get('#settings_authentication_facebook_secret').type('randomsecret');
+
+        cy.get('@authSectionForm').findByText('Update Settings').click();


just a suggestion and definitely not blocking, but it's a bit more robust to use cy.findByRole or cy.findByLabelText wherever possible, rather than getting by ID selector or inner text

Yes @aitchiss Nick also pointed this out to me yesterday 😊

These cypress tests were existing before my modifications, and the use of cy.get(#id/text) is quite plentiful throughout several files. We decided that making all these tests more robust was beyond the scope of this PR.

app/javascript/admin/controllers/config_controller.js

nickytonline · 2021-08-24T12:11:24Z

app/javascript/admin/controllers/config_controller.js

+      outcome.message == null
+        ? this.displaySnackbar(outcome.error)
+        : this.displaySnackbar(outcome.message);


Refactor (non-blocking): Use the nullish-coalescing operator.

Suggested change

outcome.message == null

? this.displaySnackbar(outcome.error)

: this.displaySnackbar(outcome.message);

this.displaySnackbar(outcome.message ?? outcome.error)

@nickytonline Isn't your suggestion reversed? The first one says display .error when .message == null, yours is the other way around, isn't it?

Updated it. 🙃

Today I learned! 😅 Thanks Nick.

nickytonline · 2021-08-24T12:51:49Z

app/javascript/admin/controllers/config_controller.js

+    );
+  }
+
+  async activateLightConfirmation(event) {


nitpick (potentially-blocking): It's not clear to me what activateLightConfirmation means. Is this method called activateLightConfirmation because it's not a form post with a full server-side roundtrip? From what it looks like in the context of where this is called, updateConfigurationSettings or updateSettings seems more appropriate.

I will change

rhymes · 2021-08-24T13:57:58Z

app/controllers/admin/settings/base_controller.rb

            end

            format.json do
-              render json: { error: "😭 #{result.errors.to_sentence}" }, status: :unauthorized
+              render json: { error: "❗️❗️ #{result.errors.to_sentence}" }, status: :unauthorized


I wonder how these symbols are going to be read by the screen reader. I understand they're added to catch the attention of the user since we don't have a way to differentiate between errors and non errors in Snackbar and we should have a global notification area for errors or something and that's outside the scope wrt the conversation with Pawel.

I think the Admin team should figure this one out in a future RFC/PR

cc. @aitchiss @ludwiczakpawel

We probably don't need any emojis in the error message.

I would love it if instead of emojis in the text we could have an optional svg/img icon in the snackbar to convey an error message vs a success/info message. You're right @rhymes in that screen reader behaviour with emojis can vary widely, and what it announces might not be something we intend (e.g. VO on Chrome announces 😭 as "loudly crying face", but this can be different when using a different screen reader/browser/etc)

Bonus context on how to mark-up emoji accessibly 😄

I think I agree with Nick here - removing the emojis from the error messaging. They were already in place when I started this task; I changed the crying-face emoji to the exclamation points as I thought that was more indicative of an error. However, using the emojis re not worth the a11y drawbacks.

Eventually, I would love our snackbars to be customizable in terms of color perhaps, to indicate a successful transaction, as opposed to a failed one. But that's another PR.

app/controllers/admin/settings/base_controller.rb

citizen428 · 2021-08-26T02:35:53Z

app/controllers/admin/settings/base_controller.rb

@@ -24,6 +40,8 @@ def extra_authorization_and_confirmation
      end

      def confirmation_text_valid?
+        return true unless params.key?(:confirmation)


This means that one can theoretically skip the config check for everything by removing :confirmation from the request params with a tool like Charles Proxy or Burp Suite. Not the biggest issue but still something to keep in mind.

msarit · 2021-08-30T20:17:10Z

This means that one can theoretically skip the config check for everything by removing :confirmation from the request params with a tool like Charles Proxy or Burp Suite. Not the biggest issue but still something to keep in mind.

@citizen428 In response to your comment above MK, I determined that we only use the confirmation form within the sections at admin/customization/config. I checked with Pod AdminExp, and since I'm making all config updates async, I have removed the confirmation form code and refactored accordingly.

app/controllers/admin/settings/base_controller.rb

app/controllers/admin/settings/general_settings_controller.rb

msarit · 2021-08-31T18:59:58Z

@nickytonline Ready for your re-review!

nickytonline

Thanks for making the changes @msarit! Looking forward to seeing this in production! 🚀

msarit added 30 commits April 9, 2021 10:41

schema file undelete description

52775fb

Updating with latest master

f754bb1

Updating with latest master

4049fda

Updating with latest master

0acb27a

Merge remote-tracking branch 'origin/main' into main

6decfcc

Updating with latest main

3f468b3

Updating with latest main

42a2a18

Updating with latest main

ab8c125

Updating with latest main

5662f5f

Updating with latest main

2b06350

Updating with latest main

97aeea4

Updating with latest main

1d06221

Updating with latest main

8043152

Updating with latest main

9cd2a6b

Updating with latest main

3d60e52

Updating with latest main

36abb4e

Updating with latest main

54c43b5

Updating with latest main

8db75f1

update with main

cf7ce5e

Updating with latest main

aee927b

Updating with latest main

54774e5

update with origin

3c2f884

Updating with latest main

a329aa4

Updating with latest main

9af29ae

Updating with latest main

4000cc0

update with main

e52b4f4

Updating with latest main

eabc6e9

Updating with latest main

1eb8c29

update with main

2b3fd04

update

c932873

msarit changed the title ~~Save Confirmation Flow in Admin (Part 1 of 2)~~ Save Confirmation - Admin (Part 1 of 2) Aug 23, 2021

citizen428 reviewed Aug 24, 2021

View reviewed changes

aitchiss approved these changes Aug 24, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Aug 24, 2021

nickytonline reviewed Aug 24, 2021

View reviewed changes

app/javascript/admin/controllers/config_controller.js Show resolved Hide resolved

nickytonline reviewed Aug 24, 2021

View reviewed changes

rhymes reviewed Aug 24, 2021

View reviewed changes

msarit added 2 commits August 25, 2021 09:32

Updating with latest main

7e51ad3

address PR review comments

644d7bb

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Aug 25, 2021

Fix failing CI

8a7ae5b

citizen428 reviewed Aug 26, 2021

View reviewed changes

msarit added 2 commits August 30, 2021 11:59

Updating with latest main

0ce5649

remove confirmtion_form code and refactor

bf25d9b

citizen428 reviewed Aug 31, 2021

View reviewed changes

app/controllers/admin/settings/base_controller.rb Outdated Show resolved Hide resolved

app/controllers/admin/settings/general_settings_controller.rb Outdated Show resolved Hide resolved

msarit added 3 commits August 31, 2021 10:15

Updating with latest main

444834f

invoke correct error status

cbd7130

fix failing cypress spec (being tripped by "undefined" values)

99d113d

citizen428 approved these changes Sep 1, 2021

View reviewed changes

nickytonline approved these changes Sep 2, 2021

View reviewed changes

nickytonline merged commit 877cb5c into main Sep 2, 2021

nickytonline deleted the msarit/admin-save-confirmation-pt1-light branch September 2, 2021 15:49

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Sep 2, 2021

msarit changed the title ~~Save Confirmation - Admin (Part 1 of 2)~~ Save Confirmation - Admin (Part 1 of 3) Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save Confirmation - Admin (Part 1 of 3) #14418

Save Confirmation - Admin (Part 1 of 3) #14418

msarit commented Aug 4, 2021 •

edited

citizen428 Aug 23, 2021

msarit Aug 24, 2021 •

edited

citizen428 Aug 25, 2021

msarit Aug 25, 2021

citizen428 Aug 31, 2021

aitchiss left a comment

aitchiss Aug 24, 2021

aitchiss Aug 24, 2021

msarit Aug 24, 2021 •

edited

nickytonline Aug 24, 2021 •

edited

citizen428 Aug 25, 2021

nickytonline Aug 25, 2021

msarit Aug 25, 2021

nickytonline Aug 24, 2021

msarit Aug 25, 2021

rhymes Aug 24, 2021

nickytonline Aug 24, 2021

aitchiss Aug 24, 2021

aitchiss Aug 24, 2021

msarit Aug 25, 2021

citizen428 Aug 26, 2021

msarit commented Aug 30, 2021

msarit commented Aug 31, 2021

nickytonline left a comment

Save Confirmation - Admin (Part 1 of 3) #14418

Save Confirmation - Admin (Part 1 of 3) #14418

Conversation

msarit commented Aug 4, 2021 • edited

What type of PR is this? (check all applicable)

Description

NOTES

QA Instructions, Screenshots, Recordings

UI accessibility concerns?

Added/updated tests?

[Forem core team only] How will this change be communicated?

What gif best describes this PR or how it makes you feel?

Choose a reason for hiding this comment

msarit Aug 24, 2021 • edited

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

aitchiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msarit Aug 24, 2021 • edited

Choose a reason for hiding this comment

nickytonline Aug 24, 2021 • edited

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

msarit commented Aug 30, 2021

msarit commented Aug 31, 2021

nickytonline left a comment

Choose a reason for hiding this comment

msarit commented Aug 4, 2021 •

edited

msarit Aug 24, 2021 •

edited

msarit Aug 24, 2021 •

edited

nickytonline Aug 24, 2021 •

edited