-
Notifications
You must be signed in to change notification settings - Fork 166
LG-5256: Add logging for help center troubleshooting links #5662
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
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
8ff9825
LG-5256: Add logging for help center troubleshooting links
aduth 87ec5a3
Rename help_center_article to help_center_article_url
aduth a21c1d2
Use redirect_to_and_log in redirect controller subclass
aduth bc249e9
Mark redirect controller class methods as private
aduth 86d2dd6
Fix valid_help_center_article digging
aduth ee9feb0
Consolidate logging by custom event name
aduth c6228f3
Move ReturnToSpController to Redirect module
aduth 99b0b31
Use flattened array of articles as "category/article"
aduth dd1846d
Refactor IDV troubleshooting links to use redirect path
aduth 6e65e46
Refactor MarketingSiteContext to be help center URL generator
aduth 5ccf844
Add specs for HelpCenterController
aduth 294b63e
Add specs for new MarketingSite methods
aduth 3ea3910
Restore legacy ReturnToSP routes
aduth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| module Redirect | ||
| class HelpCenterController < RedirectController | ||
| before_action :validate_help_center_article_params | ||
|
|
||
| def show | ||
| redirect_to_and_log MarketingSite.help_center_article_url(**article_params) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def validate_help_center_article_params | ||
| begin | ||
| return if MarketingSite.valid_help_center_article?(**article_params) | ||
| rescue ActionController::ParameterMissing | ||
| end | ||
|
|
||
| redirect_to root_url | ||
| end | ||
|
|
||
| def article_params | ||
| category, article = params.require([:category, :article]) | ||
| { category: category, article: article } | ||
| end | ||
| end | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module Redirect | ||
| class RedirectController < ApplicationController | ||
| PERMITTED_LOCATION_PARAMS = [:flow, :step, :location].freeze | ||
|
|
||
| private | ||
|
|
||
| def location_params | ||
| params.permit(*PERMITTED_LOCATION_PARAMS).to_h.symbolize_keys | ||
| end | ||
|
|
||
| def redirect_to_and_log(url, event: Analytics::EXTERNAL_REDIRECT) | ||
| analytics.track_event(event, redirect_url: url, **location_params) | ||
| redirect_to(url) | ||
| end | ||
| end | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| module Redirect | ||
| class ReturnToSpController < Redirect::RedirectController | ||
| before_action :validate_sp_exists | ||
|
|
||
| def cancel | ||
| redirect_url = sp_return_url_resolver.return_to_sp_url | ||
| redirect_to_and_log redirect_url, event: Analytics::RETURN_TO_SP_CANCEL | ||
| end | ||
|
|
||
| def failure_to_proof | ||
| redirect_url = sp_return_url_resolver.failure_to_proof_url | ||
| redirect_to_and_log redirect_url, event: Analytics::RETURN_TO_SP_FAILURE_TO_PROOF | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def sp_return_url_resolver | ||
| @sp_return_url_resolver ||= SpReturnUrlResolver.new( | ||
| service_provider: current_sp, | ||
| oidc_state: sp_request_params[:state], | ||
| oidc_redirect_uri: sp_request_params[:redirect_uri], | ||
| ) | ||
| end | ||
|
|
||
| def sp_request_params | ||
| @request_params ||= begin | ||
| if sp_request_url.present? | ||
| UriService.params(sp_request_url) | ||
| else | ||
| {} | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def sp_request_url | ||
| sp_session[:request_url] || service_provider_request&.url | ||
| end | ||
|
|
||
| def validate_sp_exists | ||
| redirect_to account_url if current_sp.nil? | ||
| end | ||
| end | ||
| end |
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
56 changes: 56 additions & 0 deletions
56
app/javascript/packages/document-capture/context/help-center.jsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import { createContext } from 'react'; | ||
| import { addSearchParams } from '@18f/identity-url'; | ||
|
|
||
| /** @typedef {import('react').ReactNode} ReactNode */ | ||
|
|
||
| /** | ||
| * @typedef HelpCenterURLParameters | ||
| * | ||
| * @prop {string} category | ||
| * @prop {string} article | ||
| * @prop {string} location | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {(params: HelpCenterURLParameters) => string} GetHelpCenterURL | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef HelpCenterContext | ||
| * | ||
| * @prop {string} helpCenterRedirectURL | ||
| * @prop {GetHelpCenterURL} getHelpCenterURL | ||
| */ | ||
|
|
||
| const HelpCenterContext = createContext( | ||
| /** @type {HelpCenterContext} */ ({ | ||
| helpCenterRedirectURL: '', | ||
| getHelpCenterURL: (params) => addSearchParams('', params), | ||
| }), | ||
| ); | ||
|
|
||
| HelpCenterContext.displayName = 'HelpCenterContext'; | ||
|
|
||
| /** | ||
| * @typedef HelpCenterContextProviderProps | ||
| * | ||
| * @prop {Omit<HelpCenterContext, 'getHelpCenterURL'>} value | ||
| * @prop {ReactNode} children | ||
| */ | ||
|
|
||
| /** | ||
| * @param {HelpCenterContextProviderProps} props | ||
| */ | ||
| function HelpCenterContextProvider({ value, children }) { | ||
| /** @type {GetHelpCenterURL} */ | ||
| const getHelpCenterURL = (params) => addSearchParams(value.helpCenterRedirectURL, params); | ||
|
|
||
| return ( | ||
| <HelpCenterContext.Provider value={{ ...value, getHelpCenterURL }}> | ||
| {children} | ||
| </HelpCenterContext.Provider> | ||
| ); | ||
| } | ||
|
|
||
| export default HelpCenterContext; | ||
| export { HelpCenterContextProvider as Provider }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
16 changes: 0 additions & 16 deletions
16
app/javascript/packages/document-capture/context/marketing-site.jsx
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /** | ||
| * Given a URL or a string fragment of search parameters and an object of parameters, returns a | ||
| * new URL or search parameters with the parameters added. | ||
| * | ||
| * @param {string} urlOrParams Original URL or search parameters. | ||
| * @param {Record<string, *>} params Search parameters to add. | ||
| * | ||
| * @return {string} Modified URL or search parameters. | ||
| */ | ||
| export function addSearchParams(urlOrParams, params) { | ||
| /** @type {URL|URLSearchParams} */ | ||
| let parsedURLOrParams; | ||
|
|
||
| /** @type {URLSearchParams} */ | ||
| let searchParams; | ||
|
|
||
| try { | ||
| parsedURLOrParams = new URL(urlOrParams); | ||
| searchParams = parsedURLOrParams.searchParams; | ||
| } catch { | ||
| parsedURLOrParams = new URLSearchParams(urlOrParams); | ||
| searchParams = parsedURLOrParams; | ||
| } | ||
|
|
||
| Object.entries(params).forEach(([key, value]) => searchParams.set(key, value)); | ||
|
|
||
| const result = parsedURLOrParams.toString(); | ||
| return parsedURLOrParams instanceof URLSearchParams ? `?${result}` : result; | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to me this seems more like a
buildmethod than agetmethod?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, could you explain more? Is the idea that a
get-prefixed method would only return a value directly, not derive something? I guess I've not made much a distinction on this previously, or at least would be happy to just offer up the value directly if that's what a "get" would imply.I don't feel too strongly, but more worry that we haven't made much of a distinction in the past, so an argument could be made to the point of "consistency for consistency's sake" 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my head, "get" is like "retrieve" like a load from a database or a hash or something
whereas in this case, we're not retrieving anything, we're building a new string based on some immediate values
But this is just personal style, like you said, we don't have a codebase style so I really don't feel that strongly about enforcing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I'll plan to keep it as-is for now, though perhaps separately we can think about / document naming conventions around methods.