Merge integration into deployment-portage For Upcoming Release
#948
Conversation
This file contains 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
Merge `deployment-portage` Code Changes Into `integration`
- For the edited .erb files, `"disabled": true` has been set for the email fields section of forms (copies approach used in `app/views/org_admin/users/edit.html.erb`)
- For added security, we are also removing `:email` from the .permit() update params of the corresponding controllers
- Notes regarding changes to app/controllers/registrations_controller.rb:
- Because `:email` was removed from the .permit() params, all of the `update_params[:email]`-related code needed to be addressed/removed (lines 161, 174, and 200)
- The removal of `:email` from the update params allows for the removal of a lot of other code in the file.
- The `if require_password` check in the `do_update` method is no longer needed. The comment on 197 stated `# user is changing email or password`. However, line 204-206 pointed out how this case is never actually reached for password changes (`def do_update_password` is executed instead). Since we are no longer allowing for the email to be changed either, it follows that `require_password` should always evaluate to false.
- As result, we only need the code that corresponded to the `else` statement for `if require_password` (previously lines 225-228, now 185-188)
- Also, because `require_password` is now always false, it follows that we can remove both the `require_password` arg from `def do_update` as well as the entirety of `def needs_password?`
These layout changes are being made to align with the changes in commit 74c960896 (i.e. setting `"disabled": true` to user.email in "Edit Profile")
The name of this button must match what is specified within `app/views/shared/_sign_in_form.html.erb`.
…-sso-test Fix Test / Update SSO button Name Within Test
The corresponding value for `devise.mailer.confirmation_instructions.subject` in `config/locales/translation.fr-CA.yml` specifies "Assistant PGD", but maybe it should be verified as well. Note: I don't think this is the proper way to update the locales files. Rather, I believe only `config/locales/en.yml` should be directly modified. However, #920 is currently preventing that.
This commit customises Devise's default value for the key `devise.failure.unconfirmed`. The value adds a link to the email confirmation page. Note: The values for this key in both the `...en-CA.yml` and `...fr-CA.yml` files were provided solely by me and need to be approved/finalised by The Alliance
This commit seeks to improve the UX flow for existing users with unconfirmed emails. By default, when using Devise's `:confirmable` module, confirmation instructions are sent to the user at the time of account creation. This will be perfect for newly created users, but poses some challenges for existing users. A "first category" of these existing users would be those who were created while `:confirmable` was absent from the codebase. These users are unconfirmed and a confirmation token was never generated for them. A "second category" of these existing users are those that are unconfirmed but do have an existing confirmation token. Since `:confirmable` has been absent from the codebase for years, the most recent `confirmation_sent_at` value is quite old (`Mon, 22 Feb 2021 15:18:24.000000000 UTC +00:00`). The code changes in `app/controllers/sessions_controller.rb` and `app/models/user.rb` improves the UX flow for users in the "first category". Rather than requiring the user to manually request new confirmation instructions, these changes auto-send the email when these users first attempt to sign in. `lib/tasks/dmp_assistant_upgrade.rake` enables this improved UX flow for the aforementioned "second category" of users. `def handle_unconfirmed_users_with_outstanding_invitations` queries for these unconfirmed users and then updates both `confirmation_token` and `confirmation_sent_at` to null. As a result, these users now meet the criteria for the "first category" and can also make use of the improved UX flow.
An "SSO created user" is a user whose account was created via the `Sign in with institutional or social ID` button. This occurs when the chosen SSO email is neither already linked to an existing account, nor does there exist a user account with the same email as the chosen SSO supplied email. In order to sign in via SSO, the user must provide the password/credentials for the SSO account that they have chosen. This could be considered a form of email confirmation. And because the user's account email will be set to match this SSO email, it follows that email confirmation via Devise's `:confirmable` module is redundant and can be bypassed. Taking the above reasoning into account, `app/models/user.rb` sets `confirmed_at: Time.now` for any future SSO created accounts. `lib/tasks/dmp_assistant_upgrade.rake` sets `lib/tasks/dmp_assistant_upgrade.rake` for any "existing SSO created accounts". Specifically, to be an "existing SSO created account", BOTH of the following conditions must be met: 1. An Identifier corresponding to the "openid_connect" `IdentifierScheme` exists for the user 2. For the identifier and corresponding user found in 1., the values of `identifier.created_at` and `user.created_at` are within 1 second of each other.
This commit re-allows existing users to link via SSO while signed out. This functionality was previously removed via commit e112202. The logic works as follows: 1) A user clicks the "Sign in with institutional or social ID" button 2) Within CILogon, the user chooses an email to sign in with. For this particular functionality to be used, the following conditions must both be true for the chosen email: a) It is not currently linked to any user accounts b) It matches an existing user.email in the database 3) If the matching user.email is confirmed, then the SSO email is linked to the user account and the user is signed-in. Otherwise, the user remains signed out and a flash notice is rendered indicating that the user.email requires confirmation.
`config/environments/test.rb`
- Add "SMTP From address" to mock Devise's sending of confirmation instructions email at time of account creation
`spec/factories/users.rb`
- Set `confirmed_at { Time.now }` in user factory
`spec/features/registrations_spec.rb`
- Change expected path to `root_path`. This is because the user should not be able to access `plans_path` until after they confirm their email
- Add check to verify that the confirmation-related flash message was sent
`spec/integration/openid_connect_sso_spec.rb`
- Removing `it 'does not create SSO link when user is signed out and SSO email is an existing account email'` test. Commit 965c336 ("Re-add SSO linking capability for signed-out users") updated the SSO behaviour in a manner that this test is no longer relevant.
- These changes replace the changes made to `lib/tasks/dmp_assistant_upgrade.rake` in commits 6087aee and 465d671. We are now setting `confirmed_at`, `confirmation_token`, and `confirmation_sent_at` to nil for all users. (the rake task subsequently sets `confirmed_at = Time.now` for all superusers).
This commit changes the flash message we are using for our custom UX flow. Now, rather than a custom message, we are reusing `devise.registrations.signed_up_but_unconfirmed`.
This commit is intended to resolve the following issue: #900
- `user.email` is already set as desired within `user = User.find_or_initialize_by(email: email)` - Also, the `.presence` method seems to simplify the assignment of `firstname` and `surname`. https://apidock.com/rails/Object/presence
Fix User Lookup Via SSO Email: Make Query Case-Insensitive
This commit takes the idea in commit 6087aee and applies it to users signing in via SSO. Via the addition of `app/controllers/concerns/email_confirmation_handler.rb`, some refactoring has been applied here as well.
Some refactoring was performed in `def openid_connect` to avoid adding `Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity` to `rubocop:disable`.
- These tests are meant to confirm that the desired behaviour is encountered via our custom email confirmation UX flow. - For more info, See comments for `app/controllers/concerns/email_confirmation_handler.rb`
In order to properly test the values of our customised `I18n.t` values (specifically, `I18n.t('devise.failure.unconfirmed')` and `I18n.t('devise.registrations.signed_up_but_unconfirmed')`), we need to set `I18n.default_locale` to match our app's default locale.
NOTE: These changes only set `I18n.default_locale = :'en-CA'` for the duration of the email confirmation tests. However, it may be best to apply this update globally throughout the tests.
- Rename `def missing_confirmation_instructions_handled?(user)` to `def confirmation_instructions_missing_and_handled?(user)` and add comments to where it is called for easy reference - Move `def confirmed_or_has_confirmation_token?` from User model to `EmailConfirmationHandler`. This check is only accessed by the concern, and it seems easiest to make it a private method and encapsulate the logic there.
This is necessary due to the following outstanding issue: #933 Output prior to this commit: ``` $ gem list psych *** LOCAL GEMS *** psych (5.1.2, default: 4.0.4, 3.3.4) ```
The change is not commited, but `config.disable_yaml = false` was set in `config/initializers/translation.rb`.
This reverts commit a4e5dca.
For DMP Assistant, `I18n.available_locales == [:"en-CA", :"fr-CA"]`. Thus, we are removing all locales files that do not end with `en.yml`, `en-CA.yml`, or `fr-CA.yml`.
This change resolves the following error:
```
rspec ./spec/features/modal_search_spec.rb:25
1) ModalSearchDialog Modal search opens and closes and allows user to search, select and remove items
Failure/Error: <td><%= l(plan.updated_at.to_date, formats: :short) %></td>
ActionView::Template::Error:
Translation missing: en-GB.date.formats.default
```
Because en-GB is not a locale used within DMP Assistant, commit f600ba0 removed the related en-GB files from the app.
Setting the locale to 'en-CA' unfortunately breaks the tests as well. There is an open issue that seeks to resolve this: #931
Search by `research_outputs.title` (case-insensitive)
Apply `translation:sync` to `yaml` Files and Remove Unused `locale/` + `locales/` Files
Intended to resolve this issue: #912 The following PR was used as a reference: #915 Co-Authored-By: Omar Rodriguez Arenas <[email protected]>
- Updated via `bundle exec rake translation:sync` with prod key. - These changes finalise some of the initial changes we made for testing in commit 6c41bd7
This change seeks to help prevent us from unwittingly syncing db field data to translation.io while working in certain environments (specifically test, development, and uat).
- The corresponding translation was commited via the following commit: 09e731e
Prevent Uploading of DB Fields to translation.io Within Test, Development, and UAT Environments
Add rake task for `openid_connect` / CILogon cleanup
Email Confirmation Changes
The corresponding view, `app/views/paginable/research_outputs/_index.html.erb`, uses `scope` rather than `@research_outputs`.
Fix Paginating, Sorting, and Searching Issues Within "Research Outputs" Tab
aaronskiba
changed the title
Integration into deployment-portage For Upcoming Releaseintegration into deployment-portage For Upcoming Release
|
These are both protected branches and there's a merge conflict. I'll make a copy of integration and merge that into deployment-portage. Closing this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes proposed in this PR:
Added
Changed
Email Confirmation Changes #923
Disable Updating of User Emails #917
Apply
translation:synctoyamlFiles and Remove Unusedlocale/+locales/Files #937Fixed
Fix User Lookup Via SSO Email: Make Query Case-Insensitive #924
Fixes to CILogon /
openid_connectTests #922Fix Paginating, Sorting, and Searching Issues Within "Research Outputs" Tab #938