-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat[Core]: Admin and User Metadata for transactional ressources and users (#5897) #4
Conversation
@shahmayur001 Thomas' comment on slack: |
a294e67
to
32df583
Compare
permitted_<resource>_attributes Method (from base_controller.rb)Purpose:This method is dynamically defined for each resource in the How it works:
Key Use:It ensures that admin users have additional metadata attributes, while non-admin users only get the default attributes. <method_name>_attributes Method (from api_helpers.rb)Purpose:This method dynamically creates accessor methods for each resource in the How it works:
Key Use:This method centralizes the logic for checking permissions and ensures that the appropriate set of attributes is returned based on user roles. Summary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this pr is still very hard to review, because the commits lack the information on why a certain change has been necessary. also lots of changes seem to be fixups for previous commits. this is hard to follow sometimes. I would also like to keep the list of models we introduce this into to a bare minimum. we can either add them later or (better) create a generator that allows to easily integrate it in other models if necessary.
core/lib/spree/app_configuration.rb
Outdated
@@ -209,6 +209,18 @@ class AppConfiguration < Preferences::Configuration | |||
# @return [Integer] Orders to show per-page in the admin (default: +15+) | |||
preference :orders_per_page, :integer, default: 15 | |||
|
|||
# @!attribute [rw] max_keys | |||
# @return [Integer] Maximum keys that can be allocated in customer and admin metadata column(default: +6+) | |||
preference :max_keys, :integer, default: 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this preference could be helpful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It limits the maximum amount of key value pairs.
As much xml expansion is not a problem here, we never the less given the low degree of auth needed to write practically anything create a stability issue.
Limiting it seemed reasonable. I will see to adapt the PR and Documentation accordingly as i believe it's important to have it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not convinced this is a valid addition. Can you name one real world example where this might be needed?
|
||
params[:order].delete(:customer_metadata) if params[:order] | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add specs for this and explain why this is necessary in the commit message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have specs for this https://github.com/gms-electronics/solidus/pull/4/files#diff-2ef02110b947167ef3c10ad87001d8c36b1425578b4126fd5d39dd234e4906b7R132
Basically, This prevents users from updating customer_metadata once order it is completed
:StockLocation, | ||
:StockMovement, | ||
:Variant | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be configurable via the app configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please amend the change to the introducing commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved!
stock_location: Spree::StockLocation, | ||
stock_movement: Spree::StockMovement, | ||
user: Spree::LegacyUser | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicating the constant in the api controller. Can we make this configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shahmayur001 I'd love to see this, but estimate if we can slip this in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved!
shipment: Spree::Shipment, | ||
stock_location: Spree::StockLocation, | ||
stock_movement: Spree::StockMovement, | ||
user: Spree::LegacyUser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Spree.user_class
Spree::LegacyUser
is only used in the dummy app for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shahmayur001 this one makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved!
@@ -79,6 +79,68 @@ module Spree::Api | |||
end | |||
end | |||
|
|||
context "when the user is not admin but has ability to create and update orders" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these specs also belong into the same commit as the implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved!
@@ -45,6 +50,22 @@ module Spree::Api | |||
expect(json_response).to have_attributes(attributes) | |||
end | |||
|
|||
it "allows creating payment with customer metadata but not admin metadata" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these specs also belong into the same commit as the implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved!
|
||
expect(json_response['email']).to eq user.email | ||
expect(json_response).not_to have_key('admin_metadata') | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these specs also belong into the same commit as the implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved!
extend ActiveSupport::Concern | ||
|
||
included do | ||
store :customer_metadata, coder: JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to tell active record to always use json
as column type in order to work in maria db
See AlchemyCMS/alchemy_cms@0817f1d
attribute :customer_metadata, :json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shahmayur001 makes sense, please adjust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved!
@tvdeyen can we agree on all transactional models plus users?
I think that would be good enough for now and we can later decide to change addresses as well. |
Yes |
@shahmayur001 to not throw the work, please squash commits that overwrite each other, keep an eye on the comments that @tvdeyen has made related to DB compatibility, we really missed that one and that was an excellent catch. Remove everything that is not transactional but users and in addition we remove stock movements as people can work (and probably will) push stock only from ERP and keep delivery notes outside. @tvdeyen I would keep returns / rmas for the same reasons as I think that this is a needed feature at the first place (correlation with 3rd party systems). Stock movements I believe are overkill. Can you live with that? |
Yes, we can live with that. Stock movements table can get pretty big, pretty fast. That's why we want to keep it as simple as possible.
|
@tvdeyen I can see that. @shahmayur001 so the agreed upon version is to also keep RMA / returns. |
911ba01
to
f718d86
Compare
e2d8324
to
192c365
Compare
This migration adds customer_metadata and admin_metadata to various tables
… and length Introduced validations and configurable limits for metadata attributes (`customer_metadata` and `admin_metadata`) to ensure consistency and prevent overly large or complex structures. **Implementation:** - Set default values for `customer_metadata` and `admin_metadata` to empty hashes (`{}`) for proper initialization. - Implemented validation to limit the number of keys, and the length of keys and values in metadata. - Added configuration checks to ensure metadata does not exceed the limits defined in `Spree::Config`. - Introduced preferences: `meta_data_max_keys`, `meta_data_max_key_length`, and `meta_data_max_value_length` to control metadata size: - `meta_data_max_keys`: Max keys allowed (default: 6). - `meta_data_max_key_length`: Max key length (default: 16). - `meta_data_max_value_length`: Max value length (default: 256). - Created a shared concern to handle metadata validation logic. - Added tests to ensure default values and validation logic work correctly. - Created a shared example that can be used in other specs to ensure consistent testing of metadata validation across various parts of the system.
Call the metadata concern to various models to validate and add admin and customer metadata
192c365
to
3286a68
Compare
b8ca440
to
b08d5ee
Compare
…tadata handling Update the API configuration to permit the `customer_metadata` attribute across various models, such as orders, payments, shipments, and return authorizations. This change allows better handling of customer metadata within API requests, ensuring consistent storage and validation of metadata attributes. Additionally, it updates the `preference` configurations for models to include `customer_metadata`. **Implementation:** - Added `customer_metadata` to the permitted attributes for several API resources. - Updated various `preference` settings, including `order_attributes`, `payment_attributes`, `shipment_attributes`, etc., to include `customer_metadata`. - Ensured that `customer_metadata` is correctly permitted in the API parameters for each relevant model, allowing admins and users to store custom metadata. - This change improves the flexibility of metadata management within the API, enabling dynamic handling and easier customizations.
User cannot update customer_metadata after the status of order is complete
Introduce the `admin_metadata` attribute to support dynamic metadata handling for admins. It enables configuration of metadata via API preferences, and ensures that metadata fields are properly permitted for admin users through new helper methods. **Implementation:** - Added `admin_metadata` to resources for admins. - Integrated `metadata_permit_parameters` and `metadata_api_parameters preference` in API configuration. - Implemented `permitted_<resource>_attributes` and `<method_name>_attributes` methods for dynamic metadata permissions. - Added `extract_metadata` in `line_items_controller` to handle metadata fields.
This configuration adds the ability to enable or disable metadata restrictions as needed. Following configurations are possible: - `meta_data_validation_enabled` : Enables or disables restrictions globally.
b08d5ee
to
b954491
Compare
Closes solidusio#5897
Concerns solidusio#5951
Breaking Changes
This feature breaks sqlite versions predating to December 2023, verify your requirements before migrating.
Introduction
Overview
This PR introduces the concept of metadata fields to solidus. The purpose of metadata fields is to allow customers and admins alike to provide additional information to transactions to either extend the informations contained or allow to insert additional data to correlate transactions with third party systems.
Examples for data can be:
Possible Applications on a very high level are:
The current scope of meta data are following ressources:
How is the additional information stored?
for each of the above resources two columns have been created:
The resources store the information as jsonb.
What is the access model?
Cancancan has been used to allow users to write data with the current scope just extended by the additional fields:
Testing
Extensive test cases have been added.
Detailed Overview
Types of Meta Data and usage suggestions
Overview
Meta data can be broken down in three large topics visible in the table below:
Administrative Metadata
Transactional resources are either by nature integrated with 3rd party systems (eg Payments, Billing in Italy with eInvoicing) or by operational requirement (ERP, CRMs like Salesforce,...). Meta data as an extension on admin site for all transactional resources to connect the transactional resources to any of such systems. Given that more and more countries will switch to a form of eInvoicing in the coming years meta data allows to fetch or store additional information in those transactions.
Customer Meta Data
Customer meta data allows users to customize a cart with either administrative (PO numbers,...) or customisation information (eg an URL to a picture to customize a shirt, text strings for incisions or in our case IMEIs to relate to a subscription) allowing to significantly extend the features without changing the underlying model. Fields should be accessible on front-end and backend.
Schema
Meta Data should net a reliable outcome:
"key1" : "value1" should be as applicable as "key2" : "value2", without restriction other than the quantity of storable key value pairs and limitations of the dimension of the value given that json as a format does not pose limits we should do that server side.
Future models to reserve or block certain key names might be possible.
Scopes & Arrays
Nature of the Values
The system is agnostic to the value contained. A decision has been made to store the data in additional columns in a jsonb which by now is relatively widely supported. Some preexisting installations might have to upgrade DBs, older sqlite versions dating before Jan 2024 are therefor not supported and need to be updated.
"key1" : "value1" should lead to the same behaviour as "key2" : "value2" without requiring modification inside the code. Reasonable provisions should be taken inside the code base to edit the quantities and content size of key value pairs and appropriate errors should be returned where rate or size limits are exceeded.
Administrative Meta Data
Allow appropriate roles to customize resources with private fields not end-user accessible.
R/W = Read and Write Access
Customer Meta Data
Allowing users to customize an incomplete order (or I'd say a cart) and call the information after a completed order. Admins should in an ideal case be allowed to edit those information after purchase, but I feel that can wait if technical limitations are currently in place.