-
Notifications
You must be signed in to change notification settings - Fork 10
[CRYSTAL-368]Enforce cov2 limits in requirment validations #383
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
base: main
Are you sure you want to change the base?
[CRYSTAL-368]Enforce cov2 limits in requirment validations #383
Conversation
d8fb91b
to
5516c54
Compare
class << self | ||
def call(requirements) | ||
custom_objects_v2_requirements = requirements[AppRequirement::CUSTOM_OBJECTS_V2_KEY] | ||
return [] unless custom_objects_v2_requirements |
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.
If I'm not mistaken, custom_objects_v2_requirements
contains a hash value. If we get {}
, then it won’t serve the intended purpose because an empty hash evaluates to true in a conditional statement. It would be better to use the .present?
method with unless
, or the .blank?
method with if
to handle the condition correctly.
objects = custom_objects_v2_requirements['objects'] | ||
return [] unless objects | ||
|
||
return [] unless objects.size > MAX_OBJECTS |
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.
How about replacing the unless
condition with an if
statement that returns the error? This change would make the code more readable.
# ========== OBJECTS VALIDATION ========== | ||
|
||
def validate_objects_excessive_limit(custom_objects_v2_requirements) | ||
objects = custom_objects_v2_requirements['objects'] |
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.
Could we use constant for the keys? (ex: 'objects')
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.
Why would a constant be better in this case? Curious to know more.
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.
Keys play a crucial role in validation and they are fixed. Using constants ensures consistency wherever the keys are used. Additionally, if the keys need to be updated, making changes in one place is much easier and we will have a set of keys that are important for validations.
I haven't added these comments everywhere; they apply to every place where the scenario matches. |
Could you please include the final requirements structure in the description including all the required keys? It would be very helpful during the review process. |
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.
Nice work! I'm still reviewing the PR and should be adding more comments later.
def validate_custom_objects_v2_requirements(requirements) # rubocop:disable Lint/UnusedMethodArgument | ||
# TODO: Uncomment this code | ||
# when translations for validation errors are avilable to use in CustomObjectsV2 module | ||
# CustomObjectsV2.call(requirements) | ||
[] |
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.
Could this code be updated and this PR only merged once the translations are available?
I'd recommend avoiding pushing commented out code and TODO comments for later as best practice.
def call(requirements) | ||
custom_objects_v2_requirements = requirements[AppRequirement::CUSTOM_OBJECTS_V2_KEY] |
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.
Considering SoC, how about receiving only COv2 requirements directly in this class since it doesn't need all the app requirements?
This would allow custom_objects_v2_requirements
to be renamed to simply requirements
as this class is scoped to COv2 requirements.
# lib/zendesk_apps_support/validations/requirements.rb
def validate_custom_objects_v2_requirements(requirements)
CustomObjectsV2.call(requirements[AppRequirement::CUSTOM_OBJECTS_V2_KEY])
end
class << self | ||
def call(requirements) | ||
custom_objects_v2_requirements = requirements[AppRequirement::CUSTOM_OBJECTS_V2_KEY] | ||
return [] unless custom_objects_v2_requirements |
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.
How about setting a default value for custom_objects_v2_requirements
to avoid this conditional?
def validate_objects_excessive_limit(custom_objects_v2_requirements) | ||
objects = custom_objects_v2_requirements['objects'] | ||
return [] unless objects | ||
|
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.
Similar to the comments above. How about the following to simplify it?
def validate_objects_excessive_limit(custom_objects_v2_requirements) | |
objects = custom_objects_v2_requirements['objects'] | |
return [] unless objects | |
def validate_objects_excessive_limit(objects = []) |
Then it can be called like that
validate_objects_excessive_limit(custom_objects_v2_requirements['objects'])
# or just `requirements` after renaming `custom_objects_v2_requirements` to `requirements`.
validate_objects_excessive_limit(requirements['objects'])
As there seems to be a pattern in this class, this applies to the other methods as well.
value: "The requirements.json file contains too many custom object fields of type dropdown. The current limit is %{max} fields per object. This app has %{count} fields of type dropdown for object %{object_key}." | ||
screenshot: "https://drive.google.com/file/d/1hILvdZQXZCtkQVupYXedPbnD_rNN4nwH/view?usp=sharing" | ||
- translation: | ||
key: "txt.apps.admin.error.app_build.excessive_cov2_multiselect_fields_per_object" | ||
title: "App builder job: requirements file contains too many custom object fields of type multiselect. The maximum number of fields of type multiselect per object is 5. Leave requirements.json as is (do not translate)" | ||
value: "The requirements.json file contains too many custom object fields of type multiselect. The current limit is %{max} fields per object. This app has %{count} fields of type multiselect for object %{object_key}." |
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.
Should dropdown
and multiselect
be a placeholder for these translations since they are keys in the schema? I guess we could use the same translation for both validations.
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.
dropdown
and multiselect
are indeed schema keys but I did check in acf custom fields, they were translated. Hence I decided to have two separate translations for both. Still I am open to suggestions!
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 think it can be confusing if they are translated in the error message given the value used in requirements.json
is not.
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.
Agreed! Will change it to use a single translation 👍
error = field_type == 'dropdown' ? # rubocop:disable Style/MultilineTernaryOperator | ||
:excessive_cov2_dropdown_fields_per_object : | ||
:excessive_cov2_multiselect_fields_per_object |
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 was a code smell for me that made me review the translations. This condition might not be needed when using the same translation.
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.
Yes, you are right. I did stated here the reason for two translations and hence this condition.
@digesh-parecha @mmassaki Added final requirements structure in the description including all the required keys. And I did found the missing validations for required keys in triggers, will add those in next commit. |
SELECTION_FIELD_LIMITS.map do |field_type, max_limit| | ||
validate_field_type_limit(object_fields, field_type, max_limit) | ||
end.flatten |
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.
Would #flat_map work here?
This applies to other parts of the code calling #map#flatten
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.
Yes it would work here 👍, Thanks for calling it out!
:excessive_cov2_dropdown_fields_per_object : | ||
:excessive_cov2_multiselect_fields_per_object | ||
|
||
check_collection_limits(fields_by_object, max_limit, error, field_type: field_type) |
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.
Is field_type
being used in #check_collection_limits
?
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.
yes, in method check_collection_limits
it a common helper method which has 4 standard parameters and uses the **context splat operator to accept any addition params.
You can check here
[].tap do |errors| | ||
fields_with_options.each do |field| | ||
options = field['custom_field_options'] | ||
next if options.size <= max_limit | ||
|
||
errors << ValidationError.new(:excessive_cov2_field_options, | ||
max: max_limit, | ||
count: options.size, | ||
field_key: field['key'], | ||
object_key: field['object_key']) | ||
end | ||
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.
Could this be replaced with #filter_map to simplify?
[].tap do |errors| | |
fields_with_options.each do |field| | |
options = field['custom_field_options'] | |
next if options.size <= max_limit | |
errors << ValidationError.new(:excessive_cov2_field_options, | |
max: max_limit, | |
count: options.size, | |
field_key: field['key'], | |
object_key: field['object_key']) | |
end | |
end | |
fields_with_options.filter_map do |field| | |
options = field['custom_field_options'] | |
if options.size > max_limit | |
ValidationError.new(:excessive_cov2_field_options, | |
max: max_limit, | |
count: options.size, | |
field_key: field['key'], | |
object_key: field['object_key']) | |
end |
[].tap do |errors| | ||
fields_with_options.each do |field| | ||
options = field['custom_field_options'] | ||
next if options.size <= max_limit |
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.
Could options
be nil
here?
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.
Yes it can be nil, and can throw the validations error which exceeding limit. Thanks for pointing this out.
[].tap do |errors| | ||
triggers.each do |trigger| | ||
next unless trigger['actions'] | ||
|
||
actions = trigger['actions'] | ||
next if actions.size <= MAX_ACTIONS_PER_TRIGGER | ||
|
||
errors << ValidationError.new(:excessive_custom_objects_v2_trigger_actions, | ||
max: MAX_ACTIONS_PER_TRIGGER, | ||
count: actions.size, | ||
trigger_title: trigger['title']) | ||
end | ||
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.
Same as above. How about using #filter_map
?
[].tap do |errors| | ||
grouped_items.each do |object_key, items| | ||
next unless items.size > max_limit | ||
|
||
errors << ValidationError.new(error, | ||
max: max_limit, | ||
count: items.size, | ||
object_key: object_key, | ||
**context) | ||
end | ||
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.
Same as above. How about using #filter_map
?
errors << ValidationError.new(error, | ||
max: max_limit, | ||
count: items.size, | ||
object_key: object_key, | ||
**context) |
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.
Could object_key
be nil
here when the object_key
is missing in fields and triggers? If so, should this be excluded from this validation given they should be flagged by the require key validation?
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.
Yes, object_key can be nil
when it is not defined. It will be flagged by schema validation, hence can be skipped here. Thanks for pointing it out!
%w[all any].sum { |key| conditions[key]&.size || 0 } | ||
end | ||
|
||
def check_collection_limits(grouped_items, max_limit, error, **context) |
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.
Should this method be called validate_colection_limits
to be consistent with the other methods in this class?
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.
Yes, it should be, thanks for pointing it out 👍
💐
/cc @zendesk/wattle
Description
The PR implements validation limits on Custom Objects V2 requirements in ZAS
Custom Objects V2 requirements = Custom Objects v2 + custom object fields +custom object triggers
Requirement structure with required fields:
New Changes Introduced
custom_objects_v2
as a valid type in requirement.jsonCustomObjectsV2
Validation Limits Enforced
Object Limits
Field Limits
Trigger Limits
Payload Size Limits
Validation Flow
References
JIRA
RFC
Before merging this PR
Risks
No, it cannot affect apps rendering for user because code changes in this PR are not yet used for app validations. We'll uncomment the once translations for validation errors are available.
medium: Right now, it does not touch any existing feature. We are introducing a new
resource_type
which is not yet used in ZAM or any of the existing apps.