Add Make task to validate asset strings by ironman5366 · Pull Request #3936 · 18F/identity-idp

ironman5366 · 2020-07-16T20:25:07Z

New JS code added in #3909 and others require strings configured in assets.js.erb and i18n-strings.js.erb.

The added make task, check_asset_strings, assets that for each such string referenced in a Javascript file, there exists a corresponding entry in one of the erb files

aduth · 2020-07-16T20:31:43Z

lib/asset_checker.rb

Probably worth making this capture lazier:

Suggested change

missing_translations = find_missing(data, /\Wt$['"](.*)['"]$/, @translation_strings)

missing_translations = find_missing(data, /\Wt$['"](.*?)['"]$/, @translation_strings)

Before: https://regex101.com/r/jW0NiC/1
After: https://regex101.com/r/jW0NiC/2

See: https://www.rexegg.com/regex-quantifiers.html#lazy_solution

I see your point with that error, but that also leaves the issue of only matching one translation per line. For completeness, how about something like \Wt\s?$['"]([^'^"]*)['"]$

https://regexr.com/58kjk

leaves the issue of only matching one translation per line

Would it? Based on the implementation below using scan, it seems like it should match multiple within the same line.

$ irb irb(main):001:0> "const x = t('one') + t('two');".scan /\Wt$['"](.*?)['"]$/ => [["one"], ["two"]]

Not sure - I was basing it off of the regex101 link you left.

Regardless, the new match group of "anything that's not a quote", is probably more robust anyway. And if we get to the point where there are escaped quotes in the names of translations, there are probably bigger problems in the codebase haha

aduth · 2020-07-16T20:36:36Z

lib/asset_checker.rb

Might be a good use-case for select:

Suggested change

missing = []

strings = file_data.scan pattern

strings.each do |s|

missing.push(s) unless source.include? s

end

missing

strings = file_data.scan pattern

strings.select { |s| ! source.include? s }

https://ruby-doc.org/core-2.4.1/Array.html#method-i-select

I think a reject would be better, but I'm happy to oneline it if that's desirable.

strings.reject { |s| source.include? s }

reject is even better, I agree 👍

aduth · 2020-07-16T20:42:57Z

lib/asset_checker.rb

Depending how this checker could be integrated into e.g. continuous integration, might it be worth raising an exception, or otherwise ensuring that the script exits with a non-zero exit code, so that it can be treated as a failure?

**Why**: Asset paths include a fingerprint which can't be known to the client unless provided by the server. Co-authored-by: Will Beddow <william.beddow@gsa.gov>

**Why**: Current Login.gov pages will check for and initialize any accordions present at page load. Since React components can be mounted after page load, and also need to initialize, there should be internal checks to protect against double initialization, since otherwise event handlers may be triggered multiple times.

**Why**: As an abstraction to simply render an image at the known asset path, encapsulating behavior to perform asset lookup by asset context

**Why**: If classList.js is imported (directly or indirectly) through any tested files, an error will occur. See inline comment for additional details.

**Why**: To recreate the existing document capture flow, we will need to be able to represent collapsible accordion content in React

**Why**: Recreating the existing document capture flow requires integrating existing tips content

lib/asset_checker.rb

zachmargolis · 2020-07-20T16:59:16Z

lib/asset_checker.rb

+  end
+
+  def self.check_file(file)
+    data = File.open(file).read


I personally prefer:

data = File.read(file)

but not a huge deal

lib/asset_checker.rb

zachmargolis

One more suggestion, not required, but otherwise LGTM

lib/asset_checker.rb

aduth · 2020-07-22T12:27:55Z

Not a blocker, and possibly as a future enhancement: It would be nice to have the inverse of this as well, to flag entries in i18n-strings.js.erb which aren't used in any files (and should therefore be removed).

ironman5366 requested a review from aduth July 16, 2020 20:25

ironman5366 marked this pull request as ready for review July 16, 2020 20:25

aduth approved these changes Jul 16, 2020

View reviewed changes

aduth and others added 6 commits July 16, 2020 16:46

Create application asset context

811f812

**Why**: Asset paths include a fingerprint which can't be known to the client unless provided by the server. Co-authored-by: Will Beddow <william.beddow@gsa.gov>

Add Image component

a8ea3cf

**Why**: As an abstraction to simply render an image at the known asset path, encapsulating behavior to perform asset lookup by asset context

Stub self global in test environment

deb624c

**Why**: If classList.js is imported (directly or indirectly) through any tested files, an error will occur. See inline comment for additional details.

Add Accordion React component

5ade1bd

**Why**: To recreate the existing document capture flow, we will need to be able to represent collapsible accordion content in React

Add document tips accordion to document capture

758e4b3

**Why**: Recreating the existing document capture flow requires integrating existing tips content

aduth force-pushed the aduth-document-tips branch from 4da514c to 758e4b3 Compare July 16, 2020 20:53

ironman5366 added 3 commits July 16, 2020 16:54

Make task to check existence of asset strings

8786858

Rubocop fixes

ecc0087

Fix YAML

b14cf5f

aduth force-pushed the will-asset-strings branch from 23e6edd to b14cf5f Compare July 16, 2020 20:56

Review changes

c810130

jmhooper approved these changes Jul 20, 2020

View reviewed changes

zachmargolis reviewed Jul 20, 2020

View reviewed changes

aduth force-pushed the aduth-document-tips branch from 1747971 to 6bdf517 Compare July 20, 2020 17:34

Base automatically changed from aduth-document-tips to master July 21, 2020 15:55

ironman5366 added 3 commits July 21, 2020 14:38

More review changes

ad88fe6

Merge branch 'master' into will-asset-strings

70c9e1f

Linter fix

875e0b5

zachmargolis approved these changes Jul 21, 2020

View reviewed changes

lib/asset_checker.rb Outdated Show resolved Hide resolved

Move exit code to check-assets

ed6f631

ironman5366 added 4 commits July 22, 2020 10:12

Add tests for asset task, and flatten strings for consistency

d57af47

Linter fixes

d0a7477

More linter fixes

256f2fe

Change syntax to appease linter

eb4062e

ironman5366 merged commit f77f2b8 into master Jul 22, 2020

ironman5366 deleted the will-asset-strings branch July 22, 2020 21:54

aduth mentioned this pull request Aug 24, 2020

Show spinner when starting document capture before Acuant script loaded #4113

Merged

	missing_translations = find_missing(data, /\Wt\(['"](.*)['"]\)/, @translation_strings)
	missing_translations = find_missing(data, /\Wt\(['"](.*?)['"]\)/, @translation_strings)

-    missing = []
-    strings = file_data.scan pattern
-    strings.each do |s|
-      missing.push(s) unless source.include? s
-    end
-    missing
+    strings = file_data.scan pattern
+    strings.select { |s| ! source.include? s }

Conversation

ironman5366 commented Jul 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironman5366 Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironman5366 Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduth commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ironman5366 Jul 17, 2020 •

edited

Loading

ironman5366 Jul 17, 2020 •

edited

Loading