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

Add support for externalised attachments #353

Merged
merged 17 commits into from
Jun 21, 2024

Conversation

davidjgoss
Copy link
Contributor

@davidjgoss davidjgoss commented Jun 16, 2024

🤔 What's changed?

This PR adds support for externalised attachment content.

Where an attachment message has a populated url field, the <Attachment/> component will now handle that accordingly depending on the media type. The specific variants are:

  • For binary/unknown attachments (including PDF etc), a download link will be shown pointing to the url
  • For image or video attachment, the url will populate the src attribute of the appropriate element
  • For text/JSON attachments, we'll attempt to fetch the content from the url and render it as normal
    • If there's an error, we'll show a message advising to check your browser console

Also included are a general refactor of <Attachment/> internals to use a more React-y component structure, the addition of an error boundary to handle any errors that happen for a given attachment, and a simpler visual treatment for text/x.cucumber.log+plain attachments:

image

⚡️ What's your motivation?

cucumber/html-formatter#281 (comment)

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

The next step after this will be to modify the HTML formatter implementation(s) to support externalising some kinds of attachment. I'll probably start with JS.

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

Copy link

netlify bot commented Jun 16, 2024

Deploy Preview for cucumber-react-components ready!

Name Link
🔨 Latest commit 07bdc16
🔍 Latest deploy log https://app.netlify.com/sites/cucumber-react-components/deploys/667536e140870b0008996818
😎 Deploy Preview https://deploy-preview-353--cucumber-react-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mpkorstanje mpkorstanje self-assigned this Jun 19, 2024
@mpkorstanje
Copy link
Contributor

What happens when resources can't be fetched?

@davidjgoss
Copy link
Contributor Author

What happens when resources can't be fetched?

You get an error shown in place of only that attachment - doesn't affect the rest of the step. See this story https://deploy-preview-353--cucumber-react-components.netlify.app/?story=gherkin--attachment--externalised-error

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jun 19, 2024

Mmh. That error message isn't very helpful. I think it would be good to add some detail to ensure people don't come to us and ask us why their attachments can't be loaded.

For example, Jenkins is commonly used and has rather restrictive CORS settings. Unless configured correctly reports with attachments are always problematic.

So two scenarios that are important to be handled nicely.

  • Someone loads the html report from the file system. And the attachment files have been removed. Is it clear that the attachment isn't there?

  • Someone loads the report from a server that can't or won't serve the files. Can we make it clear that the server didn't allow the request and possibly show the servers response (http code, responses body).

@davidjgoss
Copy link
Contributor Author

Okay, good feedback, let me see if I can make that better.

@davidjgoss davidjgoss merged commit cb8feff into main Jun 21, 2024
6 checks passed
@davidjgoss davidjgoss deleted the feat/externalised-attachments branch June 21, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants