Load all project drops and parse liquid async #1262
Conversation
aapomm
changed the title
Liquid/async
| @@ -9,14 +9,15 @@ | |||
| visit @path | |||
| end | |||
|
|
|||
| it 'dynamically renders item properties' do | |||
| it 'dynamically renders item properties', js: true do | |||
| expect(page).to have_no_css('span.text-nowrap', text: 'Loading liquid dynamic content', wait: 10) | |||
There was a problem hiding this comment.
10 seconds might be a bit long to wait, have you tested it with less? We may be able to get away with a shorter wait time
There was a problem hiding this comment.
10 seconds is only the maximum wait time and it won't really take this long. We're only setting the value here because 2 seconds might be too short and we need to account for possible hitches in CI that could cause a false negative.
| def initialize(project) | ||
| attr_accessor :project, :text | ||
|
|
||
| def initialize(project:, text: nil) | ||
| @project = project |
There was a problem hiding this comment.
What about passing the project_id instead and finding it here?
There was a problem hiding this comment.
I'm not sure if we're winning with this change if we have to update every instance of this initialization as well as possibly move the can? check. See liquid_enabled_resource:
def project_assigns
# This is required because we may be in Markup#preview that's passing
# :project_id for Tylium rendered editors
project = Project.find(params[:project_id])
authorize! :use, project
LiquidAssignsService.new(project: project, text: params[:text]).assigns
The check requires a reference to the current_user
aapomm
changed the title
| # This method uses Liquid::VariableLookup to find all liquid variables from | ||
| # a given text. We use the list to know which project assign we need. | ||
| def assigns_from_content | ||
| return AVAILABLE_PROJECT_ASSIGNS if text.nil? |
There was a problem hiding this comment.
should the default be [] rather than a full list?
There was a problem hiding this comment.
There is a case in Word where LiquidAssignsService is invoked globally and there is no text passed (report_processor_job.rb#L25 and the word thorfile Line 51. In those cases, we need to pass all the assigns available.
| variable_lookup = Liquid::VariableLookup.parse(text) | ||
| return (variable_lookup.lookups & AVAILABLE_PROJECT_ASSIGNS) |
There was a problem hiding this comment.
seeing this code and the intersect/validation, is L25 really needed? How does VariableLookup behave on empty input?
There was a problem hiding this comment.
See comment above. If the :text param is not passed, we need to fetch all the assigns.
| def cached_drops(records, record_type) | ||
| return [] if records.empty? | ||
|
|
||
| cache_key = "liquid-project-#{project.id}-#{record_type.pluralize}:#{records.maximum(:updated_at).to_i}-#{records.count}" |
There was a problem hiding this comment.
it seems we're working hard here to craft a unique key. Have you looked into how the Rails code does this when you pass a collection in your view cache call? Maybe there's a method we can use that takes a prefix and a collection and spits the unique cache key?
There was a problem hiding this comment.
Looks like rails is using the cache_key to uniquely identify each record and use that to individually cache each rendered view in the collection as mentioned here: https://guides.rubyonrails.org/caching_with_rails.html#collection-caching. Unfortunately, the method doesn't work for us as we're caching whole collections. For example, Issue.all.cache_key doesn't change if an issue record is updated/deleted/added.
There was a problem hiding this comment.
This is what I meant:
https://api.rubyonrails.org/classes/ActiveSupport/Cache.html#method-c-expand_cache_key
Spec
Currently, only the document properties, project, and team are available as global drops. This prevents us from making full use of Liquid in content and cases where a user may want to iterate over the project's issues in a content block is just not possible.
Parsing Liquid inline will start to have a performance penalty considering that we'll be pulling the project records for every #show page load, so we need the following changes:
Proposed solution
Update the LiquidAssignsService class with all the possible drops available in a project. This task includes:
Other Information
If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.
Thanks for contributing to Dradis!
Copyright assignment
Collaboration is difficult with commercial closed source but we want
to keep as much of the OSS ethos as possible available to users
who want to fix it themselves.
In order to unambiguously own and sell Dradis Framework commercial
products, we must have the copyright associated with the entire
codebase. Any code you create which is merged must be owned by us.
That's not us trying to be a jerks, that's just the way it works.
Please review the CONTRIBUTING.md
file for the details.
You can delete this section, but the following sentence needs to
remain in the PR's description:
Check List