Skip to content

LG-356 Add help text to the account creation screen for SAM#2230

Merged
stevegsa merged 1 commit intomasterfrom
stevegsa-help-text-for-sam-account-creation
Jun 15, 2018
Merged

LG-356 Add help text to the account creation screen for SAM#2230
stevegsa merged 1 commit intomasterfrom
stevegsa-help-text-for-sam-account-creation

Conversation

@stevegsa
Copy link
Contributor

@stevegsa stevegsa commented Jun 11, 2018

Why: To give SAM users clear instructions upon account creation

How: Conditionally show the sp alert partial and change it's contents for SAM

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

  • For DB changes, check for missing indexes, check to see if the changes
    affect other apps (such as the dashboard), make sure the DB columns in the
    various environments are properly populated, coordinate with devops, plan
    migrations in separate steps.

  • For route changes, make sure GET requests don't change state or result in
    destructive behavior. GET requests should only result in information being
    read, not written.

  • For encryption changes, make sure it is compatible with data that was
    encrypted with the old code.

  • For secrets changes, make sure to update the S3 secrets bucket with the
    new configs in all environments.

  • Do not disable Rubocop or Reek offenses unless you are absolutely sure
    they are false positives. If you're not sure how to fix the offense, please
    ask a teammate.

  • When reading data, write tests for nil values, empty strings,
    and invalid formats.

  • When calling redirect_to in a controller, use _url, not _path.

  • When adding user data to the session, use the user_session helper
    instead of the session helper so the data does not persist beyond the user's
    session.

  • When adding a new controller that requires the user to be fully
    authenticated, make sure to add before_action :confirm_two_factor_authenticated.

@monfresh
Copy link
Contributor

I thought we already had this enabled on the account creation screen for all SPs, but I guess not. Is there a reason why we wouldn't want this on the account creation screen for all SPs?

@stevegsa
Copy link
Contributor Author

I spoke to Vishal. The reason we are only doing it for SAM is because the wording is specific to them vs say CBP/TTP (which doesn't need to use the same email address). So rather than require usajobs and ttp, etc. to provide new wording for this screen we are hardcoding it to the one that needs this feature now.

@monfresh
Copy link
Contributor

Isn't that why we have SP-specific entries in the YAML file, which allows us to customize the message for each agency? The wording is already correct for the other agencies. Basically, what I'm saying is we already have text in place that shows up for all agencies on the sign_up/start page, and we want the same message on the "enter your email" page.

@vishaliyer-18f
Copy link

The intent and content of the messaging in the create account state is different from the landing state & sign in. There is some overlap, but they are essentially different.

In TTP for e.g, we have "Your old GOES userID and password won’t work. Please create a login.gov account below." for landing & sign in. This doesn't really apply for create account because they are already doing that. So, there's no need for any additional text for CBP in the create account state.

@monfresh
Copy link
Contributor

Yes, but for USAJOBS, it is relevant since we want to make sure they use the same email address. From an engineering perspective, instead of adding conditionals to the view, I would add the appropriate entries in the YAML file and leave them blank for those agencies that don't need any messaging on the account creation screen.

@stevegsa
Copy link
Contributor Author

I spoke to Joel about refactoring this code in general before I changed it. Specifically we want to take out all references in the code to SPs (ie SP_ALERTS) and have it data driven. For now I just added to the mess rather than refactoring the whole thing.

@stevegsa
Copy link
Contributor Author

If you would prefer I could make that one conditional dependent on if the text is populated and not have the hardcoded sp. However this code still needs to be refactored.

@stevegsa
Copy link
Contributor Author

The "if current_page"... code also needs to go.

@stevegsa stevegsa force-pushed the stevegsa-help-text-for-sam-account-creation branch from 7355a17 to 0727b2e Compare June 12, 2018 16:55
@vishaliyer-18f
Copy link

@monfresh, you are correct for USAJOBS. The desired goal is to enable-

  1. Different copy from landing/ sign in states
  2. The text being shown only for some SP's on the create account page.

@stevegsa
Copy link
Contributor Author

I went ahead and added the code to conditionally show the help text on the account creation screen if they supply it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not create an empty create_account_page.body entry for those SPs that don't need one? This will eliminate these conditionals which are hard to read IMO. It is generally considered a bad practice to have conditionals in views. We prefer the "tell don't ask" principle, such as in our heavy use of presenters, which could be another option here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would imply creating a blank entry for every sp in the yml file as well as every future sp and translation when on average it will not be supplied. I agree that it's bad style to put these conditionals in the view but the conditionals were there before I got here. The correct way to do this would be to pull out all the sp specific code from the views and session decorator (we already have the issuer which is unique) and drive it with some data configuration that feeds the decorated session which feeds the view. You are proposing to use the locale files as a mechanism to configure what gets shown. I can do it but I'd prefer 2 lines of code over dozens of unused lines in the locale files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

Copy link
Contributor Author

@stevegsa stevegsa Jun 12, 2018

Choose a reason for hiding this comment

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

I'm not totally against it. At least it makes it more data driven. I think the specs would need to be updated because they are testing for an alert box. I'm not sure if the alert box is invisible or not when there is no text.

Copy link
Contributor

@monfresh monfresh Jun 12, 2018

Choose a reason for hiding this comment

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

Why do the French and Spanish translations refer to "primary or secondary" email address, whereas the English text does not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was supposed to use the text from account_page.body for create_account_page.body but it appears the account_page.body for spanish and french are currently wrong in production. I need new translations for both.

@stevegsa stevegsa force-pushed the stevegsa-help-text-for-sam-account-creation branch 2 times, most recently from 41df549 to 02ae688 Compare June 12, 2018 20:49
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use this instead: Votre ancien nom d'utilisateur et mot de passe SAM ne marchera pas. Veuillez créer un nouveau compte login.gov avec la même adresse e-mail que vous avez utilisée pour SAM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use this text: Veuillez créer un compte login.gov avec la même adresse e-mail que vous avez utilisée pour SAM.

Copy link
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

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

LGTM % french translations

**Why**: To give SAM users clear instructions upon account creation

**How**: Conditionally show the sp alert partial and change it's contents for SAM
@stevegsa stevegsa force-pushed the stevegsa-help-text-for-sam-account-creation branch from 14a9f30 to d99cad5 Compare June 15, 2018 00:19
@stevegsa stevegsa merged commit fb63670 into master Jun 15, 2018
@amathews-fs amathews-fs deleted the stevegsa-help-text-for-sam-account-creation branch January 7, 2021 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants