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.

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

Load all project drops and parse liquid async #1262

Merged
merged 22 commits into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
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
3 changes: 1 addition & 2 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
[v#.#.#] ([month] [YYYY])
- [entity]:
- [future tense verb] [feature]
- Liquid: Make project-level collections available for Liquid syntax
- Upgraded gems: nokogiri, rails, rexml
- Bugs fixes:
- [entity]:
Expand Down
1 change: 1 addition & 0 deletions app/assets/javascripts/tylium.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
//= require tylium/modules/export
//= require tylium/modules/fileupload
//= require tylium/modules/issues
//= require tylium/modules/liquid_async
//= require tylium/modules/nodes
//= require tylium/modules/search
//= require tylium/modules/sidebar
Expand Down
22 changes: 22 additions & 0 deletions app/assets/javascripts/tylium/modules/liquid_async.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
document.addEventListener('turbolinks:load', function () {
$('[data-behavior~=liquid-async]').each(function () {
const that = this,
data = { text: $(that).attr('data-content') },
$spinner = $(that).prev().find('[data-behavior~=liquid-spinner');

fetch($(that).attr('data-path'), {
method: 'POST',
headers: {
Accept: 'text/html',
'Content-Type': 'application/json',
'X-CSRF-Token': $('meta[name="csrf-token"]').attr('content'),
},
body: JSON.stringify(data),
})
.then((response) => response.text())
.then(function (html) {
$(that).html(html);
$spinner.addClass('d-none');
});
});
});
1 change: 1 addition & 0 deletions app/assets/stylesheets/tylium/modules.scss
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@
}

.header-underline {
align-items: center;
border-bottom: 1px solid $borderColor;
display: flex;
margin-bottom: 0.5em;
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/concerns/liquid_enabled_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,6 @@ def project_assigns
project = Project.find(params[:project_id])
authorize! :use, project

LiquidAssignsService.new(project).assigns
LiquidAssignsService.new(project: project, text: params[:text]).assigns
end
end
6 changes: 3 additions & 3 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module ApplicationHelper # :nodoc:
def markup(text, options={})
def markup(text, options = {})
return unless text.present?

context = {}
Expand Down Expand Up @@ -38,8 +38,8 @@ def render_view_hooks(partial, locals: {}, feature: :addon)
;nil
end

def spinner_tag(spinner_class: 'text-primary')
content_tag :div, class: 'd-flex align-items-center justify-content-center spinner-container' do
def spinner_tag(spinner_class: 'text-primary', align: 'center', inline: false)
content_tag :div, class: "#{inline ? 'd-inline-flex' : 'd-flex' } align-items-center justify-content-#{align} spinner-container" do
content_tag :div, nil, class: "spinner-border #{spinner_class}"
end
end
Expand Down
49 changes: 46 additions & 3 deletions app/services/liquid_assigns_service.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
class LiquidAssignsService
attr_accessor :project
AVAILABLE_PROJECT_ASSIGNS = %w{ evidences issues nodes notes tags }.freeze

def initialize(project)
attr_accessor :project, :text

def initialize(project:, text: nil)
@project = project
Copy link
Contributor

Choose a reason for hiding this comment

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

What about passing the project_id instead and finding it here?

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'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

@text = text
end

def assigns
result = { 'project' => ProjectDrop.new(project) }
result = project_assigns
result.merge!(assigns_pro) if defined?(Dradis::Pro)
result
end
Expand All @@ -15,4 +18,44 @@ def assigns

def assigns_pro
end

# 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?
Copy link
Member

Choose a reason for hiding this comment

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

should the default be [] rather than a full list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
Comment on lines +27 to +28
Copy link
Member

Choose a reason for hiding this comment

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

seeing this code and the intersect/validation, is L25 really needed? How does VariableLookup behave on empty input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above. If the :text param is not passed, we need to fetch all the assigns.

end

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}"
Copy link
Member

@etdsoft etdsoft Jun 11, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

drop_class = "#{record_type.camelize}Drop".constantize

Rails.cache.fetch(cache_key) do
records.map { |record| drop_class.new(record) }
end
end

def project_assigns
project_assigns = { 'project' => ProjectDrop.new(project) }

assigns_from_content.each do |record_type|
records =
case record_type
when 'evidences'
project.evidence
when 'nodes'
project.nodes.user_nodes
else
project.send(record_type.to_sym)
end

project_assigns.merge!(record_type => cached_drops(records, record_type.singularize))
end

project_assigns
end
end
6 changes: 3 additions & 3 deletions app/views/evidence/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@
<div class="note-text-inner">
<h4 class="header-underline">
<span class="text-truncate" title="Evidence for this instance">Evidence for this instance</span>
<%= render partial: 'shared/liquid_loading' %>
<%= render partial: 'actions' %>
</h4>

<div class="content-textile" data-behavior="content-textile">
<%= markup(@evidence.content, liquid: true) %>
<div class="content-textile" data-behavior="content-textile liquid-async" data-path="<%= preview_project_node_evidence_path(current_project, @evidence.node, @evidence) %>" data-content="<%= @evidence.content %>" >
aapomm marked this conversation as resolved.
Show resolved Hide resolved
<%= markup(@evidence.content) %>
</div>
<div class="author-info">
<span class="ms-auto">Author: <%= @evidence.author || 'n/a' %></span>
Expand Down
5 changes: 3 additions & 2 deletions app/views/issues/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@
<div class="note-text-inner">
<h4 class="mb-4 header-underline">
<span class="text-truncate" title="<%= @issue.title %>"><%= @issue.title %></span>
<%= render partial: 'shared/liquid_loading' %>
<%= render partial: 'actions' %>
</h4>
<div class="content-textile" data-behavior="content-textile">
<%= markup(@issue.text, liquid: true) %>
<div class="content-textile" data-behavior="content-textile liquid-async" data-path="<%= preview_project_issue_path(current_project, @issue) %>" data-content="<%= @issue.text %>" >
aapomm marked this conversation as resolved.
Show resolved Hide resolved
<%= markup(@issue.text) %>
</div>
<div class="author-info">
<span class="ms-auto">Author: <%= @issue.author || 'n/a' %></span>
Expand Down
5 changes: 3 additions & 2 deletions app/views/notes/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@
<div id="<%= dom_id(@note) %>" class="note-text-inner">
<h4 class="header-underline">
<span class="text-truncate" title="<%= @note.title %>"><%= @note.title %></span>
<%= render partial: 'shared/liquid_loading' %>
<%= render partial: 'actions' %>
</h4>
<div class="content-textile" data-behavior="content-textile">
<%= markup(@note.text, liquid: true) %>
<div class="content-textile" data-behavior="content-textile liquid-async" data-path="<%= preview_project_node_note_path(current_project, @note.node, @note) %>" data-content="<%= @note.text %>" >
aapomm marked this conversation as resolved.
Show resolved Hide resolved
<%= markup(@note.text) %>
</div>
<div class="author-info">
<span class="ms-auto">Author: <%= @note.author || 'n/a' %></span>
Expand Down
5 changes: 3 additions & 2 deletions app/views/qa/issues/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<div class="note-text-inner">
<h4 class="mb-4 header-underline align-items-center">
<span class="text-truncate" title="<%= @issue.title %>"><%= @issue.title %></span>
<%= render partial: 'shared/liquid_loading' %>
<span class="actions">
<span class="action">
<%= link_to edit_project_qa_issue_path(current_project, @issue) do %>
Expand All @@ -29,8 +30,8 @@
</span>
</span>
</h4>
<div class="content-textile" data-behavior="content-textile">
<%= markup(@issue.text, liquid: true) %>
<div class="content-textile" data-behavior="content-textile liquid-async" data-path="<%= preview_project_issue_path(current_project, @issue) %>" data-content="<%= @issue.text %>" >
aapomm marked this conversation as resolved.
Show resolved Hide resolved
<%= markup(@issue.text) %>
</div>
<div class="author-info">
<span class="ms-auto">Author: <%= @issue.author || 'n/a' %></span>
Expand Down
8 changes: 8 additions & 0 deletions app/views/shared/_liquid_loading.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div
class="d-inline-flex align-items-center fs-6 fw-light"
data-behavior="liquid-spinner"
>
<span class="mx-2">-</span>
<%= spinner_tag spinner_class: 'spinner-border-sm text-primary', align: 'start', inline: true %>
<span class="ms-2 text-nowrap">Loading liquid dynamic content</span>
</div>
39 changes: 36 additions & 3 deletions spec/services/liquid_assigns_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,57 @@
RSpec.describe LiquidAssignsService do
let!(:project) { create(:project) }

let(:liquid_assigns) { described_class.new(project).assigns }
before do
node = create(:node, project: project)
issue = create(:issue, node: project.issue_library)
create(:evidence, issue: issue, node: node)
create(:note, node: node)
create(:tag)
end

describe '#project_assigns' do
context 'with the :text argument' do
LiquidAssignsService::AVAILABLE_PROJECT_ASSIGNS.each do |assign|
it "adds #{assign} to the project_assigns if present in the text" do
text = "#[Description]#\n {% for #{assign.singularize} in #{assign} %}{% endfor %}\n"
liquid_assigns = described_class.new(project: project, text: text).assigns

expect(liquid_assigns.keys).to include(assign)
end
end
end

it 'builds a hash of liquid assigns' do
expect(liquid_assigns['project'].name).to eq(project.name)
context 'without the :text argument' do
let(:liquid_assigns) { described_class.new(project: project).assigns }

it 'builds a hash of liquid assigns' do
expect(liquid_assigns['project'].name).to eq(project.name)
expect(liquid_assigns['issues'].map(&:title)).to eq(project.issues.map(&:title))
expect(liquid_assigns['evidences'].map(&:title)).to eq(project.evidence.map(&:title))
expect(liquid_assigns['nodes'].map(&:label)).to eq(project.nodes.user_nodes.map(&:label))
expect(liquid_assigns['notes'].map(&:title)).to eq(project.notes.map(&:title))
expect(liquid_assigns['tags'].map(&:display_name)).to eq(project.tags.map(&:display_name))
end
end
end

context 'with pro records', skip: !defined?(Dradis::Pro) do
let(:liquid_assigns) { described_class.new(project: project).assigns }

let!(:project) { create(:project, :with_team) }

before do
report_content = project.content_library
report_content.properties = { 'dradis.project' => project.name }
report_content.save

create(:content_block, project: project)
end

it 'builds a hash with Dradis::Pro assigns' do
expect(liquid_assigns['document_properties'].available_properties).to eq({ 'dradis.project' => project.name })
expect(liquid_assigns['team'].name).to eq(project.team.name)
expect(liquid_assigns['content_blocks'].map(&:content)).to eq(project.content_blocks.map(&:content))
end
end
end
5 changes: 3 additions & 2 deletions spec/support/liquid_shared_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


expect(find('.note-text-inner')).to have_content("Liquid: #{record.title}")
expect(find('.note-text-inner')).not_to have_content("Liquid: {{#{item_type}.title}}")
end
end

shared_examples 'liquid preview' do |item_type, node_association|

before do
@path = if node_association
polymorphic_path([:edit, current_project, record.node, record])
Expand Down
4 changes: 3 additions & 1 deletion spec/support/qa_shared_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@
visit polymorphic_path([current_project, :qa, record])
end

it 'parses liquid content' do
it 'parses liquid content', js: true do
expect(page).to have_no_css('span.text-nowrap', text: 'Loading liquid dynamic content', wait: 10)

expect(find('.note-text-inner')).to have_content("Liquid: #{record.title}")
expect(find('.note-text-inner')).not_to have_content("Liquid: {{#{item_type.to_s}.title}}")
end
Expand Down
Loading