Skip to content

Conversation

Nevelito
Copy link
Contributor

@Nevelito Nevelito commented Feb 21, 2025

Description

Fixes #3441

This PR adds a preview link for representable files. When clicked, it opens the file in a new browser tab, allowing users to preview the content directly without downloading it.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

without hover:
image

with hover:
image

video (PS: its random pdf file from university):
Screencast from 21.02.2025 14:10:29.webm

Copy link

qlty-cloud-legacy bot commented Feb 21, 2025

Code Climate has analyzed commit c0d540d and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

Thanks for working on this one, @Nevelito.

The openable option name is a bit difficult to spell. Let's rename it.

I have a few suggestions: external_preview, preview_on_click, or click_to_open. Something along these lines. Let's discuss a better option name.

Also, let's ensure that the link doesn't trigger any downloads. I've tested it, and while PDFs don't trigger a download, CSV files do.


PS

Hi @Nevelito,

After discussing this with Adrian, I’m editing my review as we noticed some statements in the issue that could be misinterpreted.

The goal is not to provide an option to enable or disable this feature. Instead, let's remove the configuration setting and make it always clickable and "openable."

Additionally, we need to ensure proper authorization is enforced—if a user cannot download the file, they should not be able to open it either.

@Nevelito Nevelito requested a review from Paul-Bob March 6, 2025 19:07
@Nevelito
Copy link
Contributor Author

Nevelito commented Mar 6, 2025

I think it is done and work like it should

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

I tested this, and it works fine with PDFs. However, when a new tab opens for CSV files, it simply triggers a download.

Let's ensure we render the link only for previewable file types. I believe representable? handles this check

@Nevelito
Copy link
Contributor Author

Hi @Paul-Bob do you have any idea why it can not find css in system spec? Locally everything passes clean.

Comment on lines 11 to 12
<% if file.representable? %>href="<%= helpers.main_app.url_for(file) %>" target="_blank" rel="noopener noreferrer"<% end %>
class="relative flex flex-col justify-evenly items-center px-2 rounded-lg border bg-white border-gray-500 min-h-24 <%= 'hover:bg-gray-100 transition' if file.representable? %>"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's important to check can_download_file? to enforce authorization properly. Otherwise, users without download permissions could still open and inspect the PDF.

@Paul-Bob
Copy link
Contributor

Hi @Paul-Bob do you have any idea why it can not find css in system spec? Locally everything passes clean.

Sometimes, tests may fail in specific environments. You can check each environment using the following command:

RAILS_VERSION=8.0 bundle exec appraisal rails-8.0-ruby-3.3.0 bundle exec rspec --color spec/system/avo/open_field_attachment_spec.rb

Replace 8.0 and 3.3.0 with the Rails and Ruby versions you want to test locally.

Let me know if this is the case or if the tests are still passing locally, even after replicating the failing environment from GitHub Actions.

@Paul-Bob
Copy link
Contributor

@Nevelito I made a small tweak to how the link is built in the .erb file. Could you take a look and adjust the test accordingly?

Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

@github-actions github-actions bot added the Stale label Mar 30, 2025
@Nevelito
Copy link
Contributor Author

I’ll try to finish my tasks in a few days—I’ve got an important new project to work on. Sorry!

@github-actions github-actions bot removed the Stale label Mar 31, 2025
Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

@github-actions github-actions bot added the Stale label Apr 16, 2025
@Paul-Bob Paul-Bob removed the Stale label Apr 16, 2025
@Nevelito
Copy link
Contributor Author

Still there is some system spec but locally it passes all on the specific ruby and rails version

image

@Paul-Bob
Copy link
Contributor

Thanks for letting me know @Nevelito

I will have a look into it during this week

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @Nevelito!

I noticed the strange behavior where the tests passed locally but failed on CI. These issues are tricky and sometimes complicate to debug.

Here's the process I followed to identify and fix it:

1. Try to Reproduce Locally

I attempted to reproduce the issue in three ways:

  • Running the specific failing test.
  • Running the test with specific Ruby and Rails versions.
  • Running the full test suite, in case some state leakage from previous tests caused the failure.

In all cases, the tests passed locally, so no useful information was gathered from this step.

2. Modify the Test

Next, I modified the test slightly to see if I could isolate the failure. I repeated the local runs after each change.

Still, everything passed locally, but CI continued to fail.

3. Debug on CI

Since the problem only appeared on CI, I started debugging directly there.

The test failure was due to a missing link on the page. To investigate, I added a print statement that listed all page links (hrefs) during the test run.

Findings:

  • Locally, the page had two links for the file: one for downloading and one for previewing.
  • On CI, only the download link was present; the preview link was missing.

I checked the conditions for rendering the preview link and found that it depends on calling representable? on the file.

To confirm, I added this debug line in the test:

puts ["user.cv.representable? ->", user.cv.representable?].inspect

From the CI logs, representable? was always returning false.

After some research, I realized this happens because Active Storage behaves differently depending on the environment configuration.

4. Solution

The simplest solution I thought of was to mock representable? to return true during the test.


I shared the full process because it might help when debugging similar, environment-specific issues in the future.

@Paul-Bob Paul-Bob added the Enhancement Not necessarily a feature, but something has improved label Apr 29, 2025
@Paul-Bob Paul-Bob changed the title feature: add open attachment without download enhancement: add open attachment without download Apr 29, 2025
@Paul-Bob Paul-Bob merged commit 1331479 into avo-hq:main Apr 29, 2025
19 of 20 checks passed
@Paul-Bob Paul-Bob changed the title enhancement: add open attachment without download enhancement: preview file attachment without download Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Not necessarily a feature, but something has improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open attachments in a new browser window for direct viewing (KampAdmin)
3 participants