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.

Sign up for GitHub

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

Fix a missing Plans column on the applications list #3995

Merged
merged 2 commits into from
Feb 3, 2025
Merged

Fix a missing Plans column on the applications list #3995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5c1e11
Fix a missing Plans column on the applications list
mayorova Jan 28, 2025
6b718f7
Make application list scenarios more explicit
mayorova Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/controllers/api/applications_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ class Api::ApplicationsController < FrontendController
include ApplicationsControllerMethods

before_action :authorize_partners
before_action :find_plans
before_action :find_service
before_action :find_plans
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This order is important for this controller, as when the plans are searched, they are scoped by the service, so the service needs to be pre-filled.

before_action :find_states, only: :index # rubocop:disable Rails/LexicallyScopedActionFilter
before_action :find_buyer, only: :create
before_action :authorize_multiple_applications, only: :create
Expand Down Expand Up @@ -42,8 +42,8 @@ def find_service
@service = accessible_services.find params[:service_id]
end

def accessible_plans
super.where(issuer: @service)
def find_plans
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overriding the find_plans in order to avoid adding .stock.
As I understand the logic, the idea is that if there is just one single plan (I guess that's not commonly used, but still), there is no need to show the Plan column, because well, it will always be the same.

But for that purpose I think it is more correct to check all plans (including the custom ones), and not just "stock" plans (non-custom).

@application_plans = accessible_plans.where(issuer: @service)
end

def initialize_new_presenter
Expand Down
4 changes: 1 addition & 3 deletions app/views/shared/applications/_listing.html.slim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@
- content_for :javascripts do
= javascript_packs_with_chunks_tag 'table_toolbar'

/ FIXME: The first condition does not make any sense but application_plans used to be accessible_plans, now it's accessible_plans.stock
/ FIXME 2: application_plans returns [] in services/:id/applications even though there are plans
- show_application_plans = application_plans.size > 0 && !master_on_premises?
- show_application_plans = application_plans.size > 1 && !master_on_premises?
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 agree that the condition didn't make sense 😅
And I think originally it was > 1, according to this, it was changed here.

I believe this PR is restoring the originally intended logic.

- service_column_visible = service.nil? && current_account.multiservice?
div class="pf-c-card"
table class="pf-c-table pf-m-grid-lg" role="grid" aria-label="Applications table" data-toolbar-props=@presenter.toolbar_props.to_json
Expand Down
32 changes: 23 additions & 9 deletions features/api/services/applications/index.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ Feature: Product > Applications

Background:
Given a provider
And a product "My API"
And another product "Another API"
And a product "My API" with no plans
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make sure that the plans created below are the only plans on the respective services.

And another product "Another API" with no plans
And the following application plans:
| Product | Name | Cost per month | Setup fee |
| My API | Cheap | 0 | 0 |
Expand Down Expand Up @@ -42,13 +42,27 @@ Feature: Product > Applications
And there should be a link to "Add an application"

Scenario: Only the current service applications are listed
When they go to product "My API" applications page
Then they should see the following table:
| Name | State | Account | Created on | Traffic on |
| Jane's Full App | live | Jane | December 13, 2023 | |
| Jane's Lite App | live | Jane | December 12, 2023 | |
| Bob's App | live | Bob | December 11, 2023 | |
When they go to product "My API" applications page
Then they should see the following table with exact columns:
| Name | State | Account | Plan | Created on | Traffic on |
| Jane's Full App | live | Jane | Expensive | December 13, 2023 | |
| Jane's Lite App | live | Jane | Cheap | December 12, 2023 | |
| Bob's App | live | Bob | Cheap | December 11, 2023 | |
When they go to product "Another API" applications page
Then they should see the following table:
Then they should see the following table with exact columns:
| Name | State | Account | Created on | Traffic on |
| Another API App | live | Bob | December 10, 2023 | |

Scenario: Plan column is hidden when the API has a single plan
When they go to product "My API" applications page
Then the table has a column "Plan"
When they go to product "Another API" applications page
Then the table does not have a column "Plan"

Scenario: Paid? column is shown when finance is enabled
When the provider has "finance" denied
And they go to product "My API" applications page
Then the table does not have a column "Paid?"
When the provider has "finance" allowed
And they go to product "My API" applications page
Then the table has a column "Paid?"
5 changes: 3 additions & 2 deletions features/step_definitions/table_steps.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
end

# TODO: can we use "has_table?" instead of this complex step?
Then "(I )(they )should see (the )following table(:)" do |expected|
Then /^(?:I |they )?should see (?:the )?following table( with exact columns)?:?$/ do |exact_columns, expected|
table = extract_table('table', 'tr:not(.search, .table_title)', 'td:not(.select), th:not(.select)')

# strip html entities and non letter, space or number characters
Expand All @@ -40,7 +40,8 @@

retries ||= 1

expected.diff! table
options = exact_columns.present? ? { surplus_col: true } : {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the surplus_col option, the comparison will fail if the actual table has an extra column that is not defined in the expected table.

expected.diff! table, options
rescue Cucumber::MultilineArgument::DataTable::Different, IndexError => error
if retries > 0
retries -= 1
Expand Down