It has the same name as the other feature. Maybe: ```suggestion Feature: Dev Portal Buyer Personal Details SSO ``` Also add some clarification in the feature description, like.. what SSO means in this context hehe.

```suggestion test 'update should succeed with current password' do ```

Do we need this? AFAIK this will make the method available in the liquid templates, but it's for internal controller use only.

This is not a custom field, is it?

This step make it a bit unclear when reading the scenario. The reader does not know what "any reference to password" means. I would imagine it means `I should not see "Password"`. I think this could be asserted this way: ``` And I should not see the fields: | hidden fields | | Current password | | New password | | Password confirmation | | Account extra hidden | ```

```suggestion And "password has changed" do ```

Completely redundant 😄. You can use `I press "Update Personal Details"`. (Or better _they press..._)

How do we know `"supersecret"` is the current password? Wouldn't it be more robust to get it from `@user`?

This scenario is redundant, try to merge it with _the buyer edits their personal details_. OR add a step to the other scenario `And should not see "Current password"`. No need to create a whole new step just to check a field.

This one is a bit weird, when is the form submitted? And why is email tested? Are there other validations? ```suggestion Given the buyer wants to edit their personal details When they submit an invalid email address # Fill form, click submit Then they should see "Email is wrong" # Whatever inline error is rendered ```

This is already tested in https://github.com/3scale/porta/pull/3201/files#diff-558ebc2ae6cdd5df8fec5bd78497c8250398015740fe302f22dc1e8cbc0dace3R58-R61

These 2 steps should be merged. The step should refer to the whole form, regardless of its set of fields.

This makes me think that current password is verified separately, whereas to me it would make for a better UX to verify everything at the same time. This way, when the password (or the wrong password) is provided, the form will render all errors at the same time: password wrong and missing fields.

```suggestion Given the provider has the following fields defined for "User": ```

Why the password is checked as a separate field? I think it should/could part of `they should be able to edit their personal details`.

```suggestion And the buyer uses the wrong current password ```

I think this scenario is missing a step: > Given the buyer wants to edit their personal details, when they do, then should not be able to do it It doesn't make a lot of sense to me. IMO there should be 2 scenarios: one when the buyer update their details (best case scenario) and another when the password validation fails. Since the current password is a required field, it can be tested in the "form is incomplete" scenario.

Maybe use _they_ to reduce repetition and verbosity ```suggestion When they edit their personal details ```

Is this comment really necessary?

```suggestion def user_params params.require(:user).permit([:current_password] + resource.special_fields + resource.defined_fields.map(&:name)) end ``` This works for me.

OK, I think that this code is quite outdated... I am not sure, but I think in the past (like, long long time ago), the admin portal (provider) users could log in to the developer portal. Now it's not the case, in fact the admin portal and developer portal are two different Rails engines (on different domains also), so there is no way to jump between one and another. Also, the users of the two portals are different, provider users can't login into the developer portal. Also, I think `arams[:origin] == "users"` does not apply here. In the admin portal it is valid, because there are two places from where you can go to the personal details update form: 1. From the menu **Account Settings > Personal > Personal Details** https://github.com/3scale/porta/blob/3017562449d3275840cae6afe338a01c1dcca899/app/views/shared/provider/navigation/account/_personal.html.slim#L4 2. From the list of the provider users **Account Settings > Users > Listing** https://github.com/3scale/porta/blob/3017562449d3275840cae6afe338a01c1dcca899/app/views/provider/admin/account/users/index.html.slim#L27 Which is where it may have `params[:origin] == "users"` In the developer portal though there is just a single entrypoint, and `origin=users` is not set anywhere. So this can be simplified by always redirecting to the same path. Now, there are two options: either stay on the Personal Details update form (`admin_account_personal_details_path`), or to go back to the users list page (`admin_account_users_path`). I am not sure which is the best here.

`succesfully` => `successfully` I think this typo was already in the provider controllers, so maybe we need to fix it there also.

I am wondering whether uppercase is really necessary. The message looks uppercase because it's transformed by CSS `text-transform: uppercase;`

Besides requiring `:user`, you should also add a whitelist of permitted params (`:username, :email, :password, :password_confirmation`). See https://api.rubyonrails.org/classes/ActionController/Parameters.html

If user not `.using_password?`, will this be disabled / hidden? How does it work in that scenario?

This looks like duplicated logic. It should be part of `update` somehow. Maybe add a validator for password if that's even possible or extend `update!` and return an error so that it can be caught with https://github.com/3scale/porta/blob/8612ff9ee902063a0e4c45dc49a4ffa2133e161b/lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb#L28-L31

What user is not using a password? Is there any test for this?

Use an instance variable to refer to the tested value across multiple steps: ```suggestion assert_equal BCrypt::Password.new(current_user.password_digest), @new_password ```

```suggestion assert has_css?('#user_username', text: current_user.username) # Not sure if this exists assert has_css?('#user_username', value: current_user.username) ```

I'm afraid this is all testing the same, IMO there are 2 possible scenarios: with and without current password. You could go the brief way, something like: ```suggestion Scenario: Buyer uses the wrong current password Given the buyer wants to edit their personal details When they change their personal details and password But they add the wrong current password Then they should not be able to edit their personal details Scenario: Buyer uses the wrong current password Given the buyer wants to edit their personal details When they change their personal details and password And they add the correct current password Then they should be able to edit their personal details ``` Or the long way, reusing the steps available, such as: ```cucumber And go to the personal details page And fill in "Username" with "Alfred" And press on "Update Personal Details" ``` (See features/step_definitions/web_steps.rb)

Preemptive review (2 in 1 bargain edition): 1. Changing the name of the method to an action makes its intentions clearer. 2. Since you're not using the `true` value this method return, if can simply return `nil`. Also, if there's a `return true`, I'd also expect the possibility of a `return false`. ```suggestion def verify_current_password return unless current_user.using_password? ```