Skip to content
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

Updating create account text #514

Merged
merged 4 commits into from Jan 17, 2019

Conversation

damianpm
Copy link
Contributor

@damianpm damianpm commented Jan 15, 2019

What this PR does / why we need it:

Update the page to create accounts in the Master Admin Portal to: Create new account

Which issue(s) this PR fixes

https://issues.jboss.org/browse/THREESCALE-1782

Special notes for your reviewer:
@thomasmaas , please review locally with Master account

Before:
screenshot_2019-01-15 accounts - new red hat 3scale api management 3

After:
screenshot_2019-01-15 accounts - new red hat 3scale api management 2

@damianpm damianpm changed the title [WIP]Updating create account text Updating create account text Jan 15, 2019
@damianpm damianpm force-pushed the THREESCALE-1782-Update-Master-Create-Accounts branch from 912bc95 to a0c5a3c Compare January 15, 2019 12:10
josemigallas
josemigallas previously approved these changes Jan 15, 2019
@@ -15,7 +15,7 @@ table class="data" id="buyer_accounts"

th class="actions"
- if current_account.master?
= link_to 'Create', new_provider_admin_account_path, title: 'Create new tenant account', class: 'action add'
= link_to 'Create', new_provider_admin_account_path, title: 'Create new account', class: 'action add'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so lets move the if - else inline as now the only difference is the path

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree 😄 It is simpler:

th class="actions"
  - if can?(:create, Account)
    = link_to 'Create', (current_account.master? ? new_provider_admin_account_path : new_admin_buyers_account_path), title: 'Create new account', class: 'action add'

And with this I am assuming that can?(:create, Account) for a master is always true, which should be, otherwise what we currently have here is already wrong 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Martouta @thomasmaas , I simplified it, can you check?

@codecov
Copy link

codecov bot commented Jan 16, 2019

Codecov Report

Merging #514 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
+ Coverage   92.69%   92.69%   +<.01%     
==========================================
  Files        2344     2344              
  Lines       75748    75764      +16     
==========================================
+ Hits        70213    70231      +18     
+ Misses       5535     5533       -2
Impacted Files Coverage Δ
app/models/contract.rb 94.92% <0%> (-0.04%) ⬇️
test/workers/zync_worker_test.rb 100% <0%> (ø) ⬆️
...nts/applications/application_deleted_event_test.rb 100% <0%> (ø) ⬆️
test/unit/contract_test.rb 100% <0%> (ø) ⬆️
...pec/acceptance/api/line_item_variable_cost_spec.rb
spec/acceptance/api/feature_spec.rb 100% <0%> (ø)
app/events/zync_event.rb 83.33% <0%> (+2.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1ab845...53871d0. Read the comment docs.

Copy link
Contributor

@Martouta Martouta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the behaviour in a risky way because right now we are not checking the case of not being a master and can?(:create, Account) being false 😞

@damianpm
Copy link
Contributor Author

This changes the behaviour in a risky way because right now we are not checking the case of not being a master and can?(:create, Account) being false 😞
@Martouta can you check again?

= link_to 'Create', new_provider_admin_account_path, title: 'Create new tenant account', class: 'action add'
- elsif can?(:create, Account)
= link_to 'Create', new_admin_buyers_account_path, title: 'Create new account', class: 'action add'
- if can?(:create, current_account)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- if can?(:create, current_account)
- if can?(:create, Account)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that @thomasmaas @Martouta , it works for a provider, but not for master

= link_to 'Create', new_provider_admin_account_path, title: 'Create new tenant account', class: 'action add'
- elsif can?(:create, Account)
= link_to 'Create', new_admin_buyers_account_path, title: 'Create new account', class: 'action add'
- if can?(:create, current_account)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- if can?(:create, Account) 😄
This means if the current_user can create from the model Account, while what there is currently in the code here means if the current_user can create the current_account 😂

@@ -16,6 +16,7 @@
end
end

can(:create, Account, &:signup_provider_possible?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the controller is doing is:

before_action :check_provider_signup_possible, :only => %i[new create]

def check_provider_signup_possible
redirect_to admin_buyers_accounts_path, alert: 'Please, create an Account Plan and a Service Plan first' unless current_account.signup_provider_possible?
end

def signup_provider_possible?
ensure_master
!!(services.default && account_plans.default && services.default.service_plans.default)
end

Imo in the case of member users we should check also user.has_permission?(:partners) but that would be changing the current behaviour so that's why I didn't do it 🤔

@damianpm damianpm merged commit 99ad639 into master Jan 17, 2019
@damianpm damianpm deleted the THREESCALE-1782-Update-Master-Create-Accounts branch January 17, 2019 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants