-
Notifications
You must be signed in to change notification settings - Fork 116
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
Release RC 129 to Production #4644
Conversation
Temporarily ignore the abbreviation attribute in agencies.yml to allow the config repo updates to be merged without breaking the IdP while we work on adding the Partnerships data model to the IdP schema. We will ultimately add the attribute to the `agencies` table and at that point remove this change. Also updates the localdev template YAML file and adds an explicit fixture file for testing, which is passed to the service object through dependency injection.
**Why**: Follow-up task from #4620 (comment)
…pture review step (#4623) * Omit unknown errors when considering to allow step continue **Why**: Active errors contains both field-specific and general messages, e.g. received from a failed validation attempt. The latter will never become unresolved by changing field values. Only disable continue if the step is invalid OR if there are field-specific errors yet to be addressed. * Add ReviewIssuesStep validator * Fix lint error
* placeholder * remove mobile intro step * Update app/javascript/packages/document-capture/components/document-capture.jsx Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * update comment and remove unnecessary text * remove new line * fix merge Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
**Why**: Reduce size of main application bundle, which is loaded in all paths. Phone validation dependencies are quite large, and are only used in the context of the intl-tel-input pack (screens with phone input).
The remote setting tooling was added at a time when `service_providers.yml` was tracked in the IdP codebase. At that time it was not possible to update the configs for a service provider without changing the IdP code and deploying a new sha. With identity-idp-config, this is no longer true. This commit removes the code since now we can modify service provider configs by updating identity-idp-config and recycling to apply the changes.
**Why**: Because after #4508 is merged, we won't be using this table. We will be using document_capture_sessions instead.
* Add image optimization lint task * Remove "data-name" attribute from SVG * Optimize images * Restore keyframes to ID card SVG **Why**: Bug with SVGO removes keyframes. Comments are ignored by default, which are necessary for allowable exceptions to SVG inline styles. SVGO's `removeComments` will preserve comments prefixed with an exclamation point. See: svg/svgo#888 See: https://github.com/svg/svgo/blob/master/plugins/removeComments.js * Compress PNG images
* remove aal field from service_provider * make remove_column reversible
* add specs * do not attempt throttling if document capture session does not exist
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.
LGTM
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.
lgtm
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.
One question about the removal of aal
from SP
@@ -0,0 +1,5 @@ | |||
class RemoveAalFromServiceProviders < ActiveRecord::Migration[6.1] | |||
def change | |||
safety_assured { remove_column :service_providers, :aal, :integer } |
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.
Won't this have the same problem on production we had on int
? Until the old instances are scaled out, they will be trying to access the aal
field which is no longer there...
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.
Yep, #4642 was brought into this branch to address that
Can we patch this on too? #4646 Even though the flag is disabled, the precence of the extra boolean attribute may mess up proofing |
**Why**: This version includes support for non-string attributes
@zachmargolis I've cherry-picked fa69ca4 onto this branch |
No description provided.