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

THREESCALE-11498: API plans features #3949

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

THREESCALE-11498: API plans features #3949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5d6b40
Rename `of_service` scope to `all_available`
jlledom Nov 28, 2024
d1b1d00
Fix API Docs to represent the actual API
jlledom Nov 29, 2024
60739a7
OAS Spec: Improve endpoint descriptions
jlledom Nov 29, 2024
6afe150
Endpoint descriptions: Feature in singular
jlledom Dec 18, 2024
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
11 changes: 4 additions & 7 deletions app/models/plan.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,10 @@ def visible
plan.has_many :plan_metrics, foreign_key: :plan_id, &Logic::MetricVisibility::OfMetricAssociationProxy
end

has_many :features_plans, :as => :plan

# FIXME: this should be a simple HABTM
# No it can't, because it is POLYMORPHIC
has_many :features, :through => :features_plans do
# returns all features owned by issuer, not only enabled by plan
def of_service
has_many :features_plans, as: :plan, dependent: :delete_all
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you add dependent: :delete_all?

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 don't remember, but it was a good reason, for sure!

It makes sense, though. features_plans is a N-N table between features and plans, and every record represents a feature enabled for a plan. It makes no sense to keep a feature enabled for a plan after the plan is removed.

Copy link
Contributor

@akostadinov akostadinov Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

how many of them can there be? We split deleting for transactions failing reason when they are too big. That's why we have the background deletion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan having more features has 88. I don't think that's usual though, probably most of them have less than 10. Also, the association between features and features_plans is marked as dependent: :delete_all as well:

porta/app/models/feature.rb

Line 17 in 908be5d

has_many :features_plans, :dependent => :delete_all

has_many :features, through: :features_plans do # Only enabled features
# returns all features available for this plan, not only enabled ones
def all_available
owner = proxy_association.owner
owner.issuer.features.with_object_scope(owner)
end
Expand Down
4 changes: 2 additions & 2 deletions app/views/api/plans/_features.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@

<tbody>

<tr class="notice" <%= "style=display:none;" unless @plan.features.of_service.empty? -%>>
<tr class="notice" <%= "style=display:none;" unless @plan.features.all_available.empty? -%>>
<td colspan="4">
<span>This plan has no features yet.</span>
</td>
</tr>

<%= render :partial => 'api/features/feature', :collection => @plan.features.of_service %>
<%= render :partial => 'api/features/feature', :collection => @plan.features.all_available %>
</tbody>
</table>
</div>
54 changes: 25 additions & 29 deletions doc/active_docs/account_management_api.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3121,7 +3121,7 @@
"/admin/api/account_plans/{account_plan_id}/features.xml": {
"get": {
"summary": "Account Plan Features List",
"description": "Returns the list of the features associated to an account plan.",
"description": "Returns the list of the features enabled for an account plan.",
"tags": [
"Account Plans"
],
Expand Down Expand Up @@ -3152,12 +3152,10 @@
"content": {}
}
}
}
},
"/admin/api/account_plans/{account_plan_id}/features/{id}.xml": {
},
"post": {
"summary": "Account Plan Features Create",
"description": "Associate an account feature to an account plan.",
"summary": "Account Plan Feature Enable",
"description": "Enables an account feature on an account plan.",
"tags": [
"Account Plans"
],
Expand All @@ -3171,15 +3169,6 @@
"schema": {
"type": "integer"
}
},
{
"name": "id",
"in": "path",
"description": "ID of the feature.",
"required": true,
"schema": {
"type": "integer"
}
}
],
"requestBody": {
Expand All @@ -3192,10 +3181,15 @@
"access_token": {
"type": "string",
"description": "A personal Access Token."
},
"feature_id": {
"type": "integer",
"description": "ID of the feature."
}
},
"required": [
"access_token"
"access_token",
"feature_id"
]
}
}
Expand All @@ -3207,10 +3201,12 @@
"content": {}
}
}
},
}
},
"/admin/api/account_plans/{account_plan_id}/features/{id}.xml": {
"delete": {
"summary": "Account Plan Features Delete",
"description": "Deletes the association of an account feature to an account plan.",
"summary": "Account Plan Feature Disable",
"description": "Disables an account feature on an account plan.",
"tags": [
"Account Plans"
],
Expand Down Expand Up @@ -3680,7 +3676,7 @@
"/admin/api/application_plans/{application_plan_id}/features.xml": {
"get": {
"summary": "Application Plan Feature List",
"description": "Returns the list of features of the application plan.",
"description": "Returns the list of features enabled on the application plan.",
"tags": [
"Application Plans"
],
Expand Down Expand Up @@ -3713,8 +3709,8 @@
}
},
"post": {
"summary": "Application Plan Feature Create",
"description": "Associates a feature to an application plan.",
"summary": "Application Plan Feature Enable",
"description": "Enables a feature on an application plan.",
"tags": [
"Application Plans"
],
Expand Down Expand Up @@ -3764,8 +3760,8 @@
},
"/admin/api/application_plans/{application_plan_id}/features/{id}.xml": {
"delete": {
"summary": "Application Plan Feature Delete",
"description": "Removes the association of a feature to an application plan.",
"summary": "Application Plan Feature Disable",
"description": "Disable a feature on an application plan.",
"tags": [
"Application Plans"
],
Expand Down Expand Up @@ -7384,7 +7380,7 @@
"/admin/api/service_plans/{service_plan_id}/features.xml": {
"get": {
"summary": "Service Plan Feature List",
"description": "Returns the list of features of a service plan.",
"description": "Returns the list of features enabled on a service plan.",
"tags": [
"Service Plans"
],
Expand Down Expand Up @@ -7417,8 +7413,8 @@
}
},
"post": {
"summary": "Service Plan Feature Add",
"description": "Associates an existing feature to a service plan.",
"summary": "Service Plan Feature Enable",
"description": "Enables an existing feature on a service plan.",
"tags": [
"Service Plans"
],
Expand Down Expand Up @@ -7468,8 +7464,8 @@
},
"/admin/api/service_plans/{service_plan_id}/features/{id}.xml": {
"delete": {
"summary": "Service Plan Features Delete",
"description": "Removes the association of a feature to a service plan.",
"summary": "Service Plan Feature Disable",
"description": "Disables a feature on a service plan.",
"tags": [
"Service Plans"
],
Expand Down
2 changes: 1 addition & 1 deletion test/integration/user-management-api/service_features_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def setup

xml = Nokogiri::XML::Document.parse(@response.body)

assert_all_features_of_service xml, service
assert_all_features_available xml, service
end

test 'show' do
Expand Down
2 changes: 1 addition & 1 deletion test/test_helpers/xml_assertions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ def assert_an_application_plan(xml, service)
assert xml.xpath('.//plan/cancellation_period').presence
end

def assert_all_features_of_service(xml, service)
def assert_all_features_available(xml, service)
assert xml.xpath('.//features/feature/service_id').presence
assert xml.xpath('.//features/feature/service_id')
.all? { |service_id| service_id.text == service.id.to_s }
Expand Down