From a397ba1ca9205bbf73024ab1ce42a7c477b78031 Mon Sep 17 00:00:00 2001 From: binarygit Date: Tue, 13 Sep 2022 16:17:39 +0545 Subject: [PATCH 1/8] Replace what's this tooltips There are tooltips here that don't have a what's this? There are many angular directives/methods being used that I haven't looked into Every select box is using select2 --- app/views/admin/enterprises/form/_users.html.haml | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app/views/admin/enterprises/form/_users.html.haml b/app/views/admin/enterprises/form/_users.html.haml index 863de9c8bf..01bfdf24a3 100644 --- a/app/views/admin/enterprises/form/_users.html.haml +++ b/app/views/admin/enterprises/form/_users.html.haml @@ -6,8 +6,7 @@ =f.label :owner_id, t('.owner') - if full_permissions %span.required * - %div{'ofn-with-tip' => t('.owner_tip')} - %a= t('admin.whats_this') + = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.owner_tip')} .eight.columns.omega - if full_permissions = f.hidden_field :owner_id, class: "select2 fullwidth", 'user-select' => 'Enterprise.owner', 'ng-model' => 'Enterprise.owner' @@ -19,8 +18,7 @@ =f.label :user_ids, t('.notifications') - if full_permissions %span.required * - %div{'ofn-with-tip' => t('.contact_tip')} - %a= t('admin.whats_this') + = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.contact_tip')} .eight.columns.omega - if full_permissions %select.select2.fullwidth{id: 'receives_notifications_dropdown', name: 'receives_notifications', ng: {model: 'receivesNotifications', init: "receivesNotifications = '#{@enterprise.contact.id}'"}} @@ -34,8 +32,7 @@ =f.label :user_ids, t('.managers') - if full_permissions %span.required * - %div{'ofn-with-tip' => t('.managers_tip')} - %a= t('admin.whats_this') + = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.managers_tip')} .eight.columns.omega - if full_permissions %table.managers @@ -66,8 +63,7 @@ .three.columns.alpha %label = t('.invite_manager') - %div{'ofn-with-tip' => t('.invite_manager_tip')} - %a= t('admin.whats_this') + = render partial: 'admin/shared/tooltip', locals: {tooltip_text: t('.invite_manager_tip')} .eight.columns.omega .row %a.button{ "data-controller": "help-modal-link", "data-action": "click->help-modal-link#open", "data-help-modal-link-target-value": "invite-manager-modal" } From 918e22055701e2b5b534b7fd153dc279adbe820b Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Fri, 10 Mar 2023 14:55:48 +0100 Subject: [PATCH 2/8] Move `$locationProvider` configuration to another file Actually the `config()` method of `admin_ofn` file did not run on `/admin/enterprises/*` pages for an unknown reason Now those two files have the same configuration --- app/assets/javascripts/admin/utils/utils.js.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/admin/utils/utils.js.coffee b/app/assets/javascripts/admin/utils/utils.js.coffee index f509d32c1f..b8e1198e76 100644 --- a/app/assets/javascripts/admin/utils/utils.js.coffee +++ b/app/assets/javascripts/admin/utils/utils.js.coffee @@ -1,2 +1,3 @@ -angular.module("admin.utils", ["templates", "ngSanitize"]).config ($httpProvider) -> +angular.module("admin.utils", ["templates", "ngSanitize"]).config ($httpProvider, $locationProvider) -> + $locationProvider.hashPrefix('') $httpProvider.defaults.headers.common["Accept"] = "application/json, text/javascript, */*" From 9c84a6936ac99de4608c9fa25bff5489e7f063d2 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Mar 2023 14:41:48 +0100 Subject: [PATCH 3/8] Create a concern for manager invitations Can be used elsewhere Update manager_invitations.rb --- .../admin/manager_invitations_controller.rb | 19 ++----------------- .../concerns/manager_invitations.rb | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 17 deletions(-) create mode 100644 app/controllers/concerns/manager_invitations.rb diff --git a/app/controllers/admin/manager_invitations_controller.rb b/app/controllers/admin/manager_invitations_controller.rb index 39ea737021..549cafbb2f 100644 --- a/app/controllers/admin/manager_invitations_controller.rb +++ b/app/controllers/admin/manager_invitations_controller.rb @@ -3,6 +3,7 @@ module Admin class ManagerInvitationsController < Spree::Admin::BaseController authorize_resource class: false + include ManagerInvitations def create @email = params[:email] @@ -18,7 +19,7 @@ module Admin return end - new_user = create_new_manager + new_user = create_new_manager(@email, @enterprise) if new_user render json: { user: new_user.id }, status: :ok @@ -27,21 +28,5 @@ module Admin status: :internal_server_error end end - - private - - def create_new_manager - password = Devise.friendly_token - new_user = Spree::User.create(email: @email, unconfirmed_email: @email, password: password) - new_user.reset_password_token = Devise.friendly_token - # Same time as used in Devise's lib/devise/models/recoverable.rb. - new_user.reset_password_sent_at = Time.now.utc - new_user.save! - - @enterprise.users << new_user - EnterpriseMailer.manager_invitation(@enterprise, new_user).deliver_later - - new_user - end end end diff --git a/app/controllers/concerns/manager_invitations.rb b/app/controllers/concerns/manager_invitations.rb new file mode 100644 index 0000000000..ea5877aca6 --- /dev/null +++ b/app/controllers/concerns/manager_invitations.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module ManagerInvitations + extend ActiveSupport::Concern + + def create_new_manager(email, enterprise) + password = Devise.friendly_token + new_user = Spree::User.create(email: email, unconfirmed_email: email, password: password) + new_user.reset_password_token = Devise.friendly_token + # Same time as used in Devise's lib/devise/models/recoverable.rb. + new_user.reset_password_sent_at = Time.now.utc + if new_user.save + enterprise.users << new_user + EnterpriseMailer.manager_invitation(enterprise, new_user).deliver_later + end + + new_user + end +end From 465a295dfac5ac13b4651e3c1c7adb14a90d4c26 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Mar 2023 14:52:08 +0100 Subject: [PATCH 4/8] Delete manager invitation controller --- .rubocop_todo.yml | 1 - .../enterprise_controller.js.coffee | 13 --- .../admin/manager_invitations_controller.rb | 32 ------- config/routes/admin.rb | 2 - .../manager_invitations_controller_spec.rb | 89 ------------------- 5 files changed, 137 deletions(-) delete mode 100644 app/controllers/admin/manager_invitations_controller.rb delete mode 100644 spec/controllers/admin/manager_invitations_controller_spec.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 9777eec6b9..deab0bfce7 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -119,7 +119,6 @@ Layout/LineLength: - 'spec/controllers/admin/bulk_line_items_controller_spec.rb' - 'spec/controllers/admin/column_preferences_controller_spec.rb' - 'spec/controllers/admin/enterprises_controller_spec.rb' - - 'spec/controllers/admin/manager_invitations_controller_spec.rb' - 'spec/controllers/admin/order_cycles_controller_spec.rb' - 'spec/controllers/admin/schedules_controller_spec.rb' - 'spec/controllers/admin/stripe_accounts_controller_spec.rb' diff --git a/app/assets/javascripts/admin/enterprises/controllers/enterprise_controller.js.coffee b/app/assets/javascripts/admin/enterprises/controllers/enterprise_controller.js.coffee index e3191a17d1..b5977a58bf 100644 --- a/app/assets/javascripts/admin/enterprises/controllers/enterprise_controller.js.coffee +++ b/app/assets/javascripts/admin/enterprises/controllers/enterprise_controller.js.coffee @@ -55,19 +55,6 @@ angular.module("admin.enterprises") else alert ("#{manager.email}" + " " + t("is_already_manager")) - $scope.inviteManager = -> - $scope.invite_errors = $scope.invite_success = null - email = $scope.newUser - - $http.post("/admin/manager_invitations", {email: email, enterprise_id: $scope.Enterprise.id}).then (response)-> - $scope.addManager({id: response.data.user, email: email}) - $scope.invite_success = t('user_invited', email: email) - .catch (response) -> - $scope.invite_errors = response.data.errors - - $scope.resetModal = -> - $scope.newUser = $scope.invite_errors = $scope.invite_success = null - $scope.removeLogo = -> $scope.performEnterpriseAction("removeLogo", "immediate_logo_removal_warning", "removed_logo_successfully") diff --git a/app/controllers/admin/manager_invitations_controller.rb b/app/controllers/admin/manager_invitations_controller.rb deleted file mode 100644 index 549cafbb2f..0000000000 --- a/app/controllers/admin/manager_invitations_controller.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -module Admin - class ManagerInvitationsController < Spree::Admin::BaseController - authorize_resource class: false - include ManagerInvitations - - def create - @email = params[:email] - @enterprise = Enterprise.find(params[:enterprise_id]) - - authorize! :edit, @enterprise - - existing_user = Spree::User.find_by(email: @email) - - if existing_user - render json: { errors: t('admin.enterprises.invite_manager.user_already_exists') }, - status: :unprocessable_entity - return - end - - new_user = create_new_manager(@email, @enterprise) - - if new_user - render json: { user: new_user.id }, status: :ok - else - render json: { errors: t('admin.enterprises.invite_manager.error') }, - status: :internal_server_error - end - end - end -end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 8bd8cf7620..b28f1eae57 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -42,8 +42,6 @@ Openfoodnetwork::Application.routes.draw do resources :tag_rules, only: [:destroy] end - resources :manager_invitations, only: [:create] - resources :enterprise_relationships resources :enterprise_roles diff --git a/spec/controllers/admin/manager_invitations_controller_spec.rb b/spec/controllers/admin/manager_invitations_controller_spec.rb deleted file mode 100644 index 0809d40399..0000000000 --- a/spec/controllers/admin/manager_invitations_controller_spec.rb +++ /dev/null @@ -1,89 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -module Admin - describe ManagerInvitationsController, type: :controller do - include OpenFoodNetwork::EmailHelper - - let!(:enterprise_owner) { create(:user) } - let!(:other_enterprise_user) { create(:user) } - let!(:existing_user) { create(:user) } - let!(:enterprise) { create(:enterprise, owner: enterprise_owner ) } - let!(:enterprise2) { create(:enterprise, owner: other_enterprise_user ) } - let(:admin) { create(:admin_user) } - - describe "#create" do - context "when given email matches an existing user" do - before do - allow(controller).to receive_messages spree_current_user: admin - end - - it "returns an error" do - spree_post :create, email: existing_user.email, enterprise_id: enterprise.id - - expect(response.status).to eq 422 - expect(json_response['errors']).to eq 'User already exists' - end - end - - context "signing up a new user" do - let(:mail_mock) { double(:mailer, deliver_later: true) } - - before do - allow(EnterpriseMailer).to receive(:manager_invitation). - with(enterprise, kind_of(Spree::User)) { mail_mock } - - allow(controller).to receive_messages spree_current_user: admin - end - - it 'enqueues an invitation email' do - expect(mail_mock).to receive(:deliver_later) - - spree_post :create, email: 'un.registered@email.com', enterprise_id: enterprise.id - end - - it "returns the user id" do - spree_post :create, email: 'un.registered@email.com', enterprise_id: enterprise.id - - new_user = Spree::User.find_by(email: 'un.registered@email.com') - expect(json_response['user']).to eq new_user.id - end - end - end - - describe "with enterprise permissions" do - context "as user with proper enterprise permissions" do - before do - setup_email - allow(controller).to receive_messages spree_current_user: enterprise_owner - end - - it "returns success code" do - spree_post :create, email: 'an@email.com', enterprise_id: enterprise.id - - new_user = Spree::User.find_by(email: 'an@email.com') - - expect(new_user.reset_password_token).to_not be_nil - expect(json_response['user']).to eq new_user.id - expect(response.status).to eq 200 - end - end - - context "as another enterprise user without permissions for this enterprise" do - before do - allow(controller).to receive_messages spree_current_user: other_enterprise_user - end - - it "returns unauthorized response" do - spree_post :create, email: 'another@email.com', enterprise_id: enterprise.id - - new_user = Spree::User.find_by(email: 'another@email.com') - - expect(new_user).to be_nil - expect(response.status).to eq 302 - end - end - end - end -end From a13227defadbaec13b8d93116ba935117135b4ef Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Mar 2023 15:02:05 +0100 Subject: [PATCH 5/8] Add I18N to all reflexes --- app/reflexes/application_reflex.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/reflexes/application_reflex.rb b/app/reflexes/application_reflex.rb index 7fc8b6cc3d..ff41bf8288 100644 --- a/app/reflexes/application_reflex.rb +++ b/app/reflexes/application_reflex.rb @@ -23,4 +23,8 @@ class ApplicationReflex < StimulusReflex::Reflex def current_ability Spree::Ability.new(current_user) end + + def with_locale(&block) + I18n.with_locale(current_user.locale, &block) + end end From ecd5033efa4cde09d9f9d266d6051bcf9b43279a Mon Sep 17 00:00:00 2001 From: binarygit Date: Mon, 3 Oct 2022 15:05:01 +0545 Subject: [PATCH 6/8] Replace angular for when adding a new unregistered manager to an enterprise Co-Authored-By: David Cook --- app/reflexes/invite_manager_reflex.rb | 43 +++++++++++++++ .../_add_new_unregistered_manager.html.haml | 22 ++++++++ .../admin/enterprises/form/_users.html.haml | 20 +------ spec/system/admin/enterprises_spec.rb | 52 +++++++++++++++++++ 4 files changed, 118 insertions(+), 19 deletions(-) create mode 100644 app/reflexes/invite_manager_reflex.rb create mode 100644 app/views/admin/enterprises/form/_add_new_unregistered_manager.html.haml diff --git a/app/reflexes/invite_manager_reflex.rb b/app/reflexes/invite_manager_reflex.rb new file mode 100644 index 0000000000..ad36d80150 --- /dev/null +++ b/app/reflexes/invite_manager_reflex.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +class InviteManagerReflex < ApplicationReflex + include ManagerInvitations + + def invite + email = params[:email] + enterprise = Enterprise.find(params[:enterprise_id]) + + authorize! :edit, enterprise + + existing_user = Spree::User.find_by(email: email) + + locals = { error: nil, success: nil, email: email, enterprise: enterprise } + + if existing_user + locals[:error] = I18n.t('admin.enterprises.invite_manager.user_already_exists') + + return_morph(locals) + return + end + + begin + new_user = create_new_manager(email, enterprise) + locals[:success] = true + locals[:email] = new_user.email + rescue StandardError => e + @error = e.message + locals[:error] = @error || I18n.t('admin.enterprises.invite_manager.error') + end + + return_morph(locals) + end + + private + + def return_morph(locals) + morph "#add_manager_modal", + with_locale { + render(partial: "admin/enterprises/form/add_new_unregistered_manager", locals: locals) + } + end +end diff --git a/app/views/admin/enterprises/form/_add_new_unregistered_manager.html.haml b/app/views/admin/enterprises/form/_add_new_unregistered_manager.html.haml new file mode 100644 index 0000000000..c24beeba83 --- /dev/null +++ b/app/views/admin/enterprises/form/_add_new_unregistered_manager.html.haml @@ -0,0 +1,22 @@ +%div#add_manager_modal + %form{ "data-reflex": "submit->InviteManager#invite", "data-reflex-serialize-form": true } + .margin-bottom-30.text-center + .text-big + = t('js.admin.modals.invite_title') + + - if success + %p.alert-box.ok= t('user_invited', email: email) + + - if error + %p.alert-box.error= error + + = text_field_tag :email, nil, class: 'fullwidth margin-bottom-20' + = hidden_field_tag :enterprise_id, @enterprise&.id || enterprise.id + + .modal-actions + - if success + %input{ class: "button icon-plus secondary", type: 'button', value: t('js.admin.modals.close'), "data-action": "click->help-modal#close" } + - else + %input{ class: "button icon-plus secondary", type: 'button', value: t('js.admin.modals.cancel'), "data-action": "click->help-modal#close" } + = submit_tag "#{t('js.admin.modals.invite')}" + diff --git a/app/views/admin/enterprises/form/_users.html.haml b/app/views/admin/enterprises/form/_users.html.haml index 01bfdf24a3..2579b35369 100644 --- a/app/views/admin/enterprises/form/_users.html.haml +++ b/app/views/admin/enterprises/form/_users.html.haml @@ -72,22 +72,4 @@ -# add to admin footer to avoid nesting invitation form inside enterprise form - content_for :admin_footer do = render HelpModalComponent.new(id: "invite-manager-modal", close_button: false) do - %div{ng: {app: 'admin.enterprises', controller: 'enterpriseCtrl'}} - - .margin-bottom-30.text-center - .text-big - = t('js.admin.modals.invite_title') - - %p.alert-box.ok{ng: {show: 'invite_success'}} - {{invite_success}} - - %p.alert-box.error{ng: {show: 'invite_errors'}} - {{invite_errors}} - - %input#invite_email.fullwidth.margin-bottom-20{ng: {model: 'newUser'}} - - .margin-bottom-20.text-center - %button.text-center.margin-top-10{ng: {show: '!invite_success', click: 'inviteManager()'}} - = t('js.admin.modals.invite') - %button.text-center.margin-top-10{"data-action": "click->help-modal#close", ng: {show: 'invite_success', click: 'resetModal();'}} - = t('js.admin.modals.close') + = render partial: 'admin/enterprises/form/add_new_unregistered_manager', locals: { error: nil, success: nil } diff --git a/spec/system/admin/enterprises_spec.rb b/spec/system/admin/enterprises_spec.rb index d4302bc462..d377f51635 100644 --- a/spec/system/admin/enterprises_spec.rb +++ b/spec/system/admin/enterprises_spec.rb @@ -532,5 +532,57 @@ describe ' end end end + + describe "check users tab" do + before do + login_as_admin_and_visit edit_admin_enterprise_path(distributor1) + within ".side_menu" do + click_link 'Users' + end + end + + context "invite user as manager" do + before do + expect(page).to have_selector('a', text: /Add an unregistered user/i) + page.find('a', text: /Add an unregistered user/i).click + end + + it "shows an error message if the email is invalid" do + within ".reveal-modal" do + expect(page).to have_content "Invite an unregistered user" + fill_in "email", with: "invalid_email" + + expect do + click_button "Invite" + expect(page).to have_content "Email is invalid" + end.to_not enqueue_job ActionMailer::MailDeliveryJob + end + end + + it "shows an error message if the email is already linked to an existing user" do + within ".reveal-modal" do + expect(page).to have_content "Invite an unregistered user" + fill_in "email", with: distributor1.owner.email + + expect do + click_button "Invite" + expect(page).to have_content "User already exists" + end.to_not enqueue_job ActionMailer::MailDeliveryJob + end + end + + it "finally, can invite unregistered users" do + within ".reveal-modal" do + expect(page).to have_content "Invite an unregistered user" + fill_in "email", with: "email@email.com" + + expect do + click_button "Invite" + expect(page).to have_content "email@email.com has been invited to manage this enterprise" + end.to enqueue_job(ActionMailer::MailDeliveryJob).exactly(:twice) + end + end + end + end end end From 8dffb38bf5afabc4dea4fb33ea7919ed10ae15ea Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Mar 2023 16:07:46 +0100 Subject: [PATCH 7/8] Sort each array to ensure the order and then expect the right values. --- spec/services/order_cycle_form_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index ec2f92e0cd..562a0b2ed6 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -288,8 +288,10 @@ describe OrderCycleForm do distributor.users.first ) - expect{ form.save }.to change{ order_cycle.distributor_payment_methods.pluck(:id) } - .from([distributor_payment_method, distributor_payment_method2]) + expect{ form.save }.to change{ + order_cycle.distributor_payment_methods.pluck(:id).sort + } + .from([distributor_payment_method, distributor_payment_method2].sort) .to([distributor_payment_method]) end end From 29cdadd56358eca2b45a4fa2a9186f6811c1972c Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Tue, 14 Mar 2023 15:56:16 +0100 Subject: [PATCH 8/8] Avoid using exception but simply errors attribute contained in object Co-Authored-By: Maikel --- app/reflexes/invite_manager_reflex.rb | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/reflexes/invite_manager_reflex.rb b/app/reflexes/invite_manager_reflex.rb index ad36d80150..b1bd4d04a2 100644 --- a/app/reflexes/invite_manager_reflex.rb +++ b/app/reflexes/invite_manager_reflex.rb @@ -20,13 +20,12 @@ class InviteManagerReflex < ApplicationReflex return end - begin - new_user = create_new_manager(email, enterprise) + new_user = create_new_manager(email, enterprise) + + if new_user.errors.empty? locals[:success] = true - locals[:email] = new_user.email - rescue StandardError => e - @error = e.message - locals[:error] = @error || I18n.t('admin.enterprises.invite_manager.error') + else + locals[:error] = new_user.errors.full_messages.to_sentence end return_morph(locals)