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

dm-4270 relay resource links #733

Merged
merged 25 commits into from
Nov 28, 2023
Merged

Conversation

PhilipDeFraties
Copy link
Collaborator

@PhilipDeFraties PhilipDeFraties commented Nov 9, 2023

JIRA issue link

https://agile6.atlassian.net/browse/DM-4270

Description - what does this code do?

  • Adds new endpoint /practice_resources/:id/download to act as relay for innovation resource file downloads, redirects to s3 download via presigned url. These links will now work when saved, however for non-public innovations the user will be required to first authenticate, if not already logged in, and in turn will redirect to a new resource download page (see screenshot) and will simultaneously open a new tab with the file (or in the case of a docx file will initiate the download).
  • Edits practice resource file links to utilize new endpoint

Testing done - how did you test it/steps on how can another person can test it

Public Innovations:

  1. Locally, as admin, nav to a practice's "Overview" edit page
  2. Add files as attachments for "Problem", "Solution", and "Results"
  3. Go to the "Implementation" edit page, add files for "Core", "Optional", and "Support" resources
  4. View the Innovation show page for the practice
  5. Verify the links for all the downloadable files work as expected, opening a new window with the downloadable file for each. Copy the url of one of the links.
  6. Make sure the practice is practice.published && practice.enabled && practice.approved && practice.published

Non-public Innovations:

  1. Sign out from the user profile, paste in the copied url and verify the file either opens in the browser (pdfs or image files) or sends the file to the browser's downloads (docx).
  2. In the rails console, update the practice: practice.update!(is_public: false
  3. paste the copied url into the browser again, verify that you are redirected to the authentication page.
  4. Log in and you should be redirected to this page in the original tab (see screenshot)
  5. Upon render a new tab will open with the file, for docx files the tab will briefly open and the file will download to the browsers downloads folder then close.
  6. From the original tab, verify the re-download and innovation page links work as expected.

Non-published Innovations:

  1. Update the practice to be un-published: `practice.update!(published: false)
  2. Log out from the user profile, paste the url in the browser once more
  3. Verify redirect to the home page with an unauthorized content message under the nav bar:
    Screenshot 2023-11-22 at 12 22 08 PM
  4. Log in as an admin paste the url in the browser, verify the file opens in a new tab.
  5. Log out and repeat previous step after logging in as the practice user or a practice editor for the given practice.
  6. While logged in view the practice page and verify the download links work as expected

Screenshots, Gifs, Videos from application (if applicable)

Screenshot 2023-11-28 at 11 50 16 AM

Link to mock-ups/mock ups (image file if you have it) (if applicable)

Acceptance criteria

  • [ ]

Definition of done

  • Unit tests written (if applicable)
  • e2e/accessibility tests written (if applicable)
  • Events are logged appropriately
  • Documentation has been updated, if applicable
  • A link has been provided to the originating JIRA issue
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

@PhilipDeFraties PhilipDeFraties changed the title Dm 4270 relay resource links dm-4270 relay resource links Nov 9, 2023
@PhilipDeFraties PhilipDeFraties marked this pull request as ready for review November 14, 2023 19:07
@PhilipDeFraties PhilipDeFraties self-assigned this Nov 14, 2023
Copy link
Contributor

@bradjohnson92008 bradjohnson92008 left a comment

Choose a reason for hiding this comment

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

Working as expected - great job Phil!

update path helper in partial for resource link
edits `set` method, changes name, adds case statement to determine resource type

adds strong params
adds `resource_type` param
changes file link to use new practice resource endpoint
adds factories for 'PracticeSolutionResource', 'PracticeResultsResource', and 'PracticeProblemResource' that inherit from `:practice_resource` factory
adds tests for additional file types
…write method

allows practice resource link for non-public practice to be protected by authentication
adds logic to allow traffic to private practices for admins only

changes download session token to hash object containing `download_url`, `practice_path` and `practice_name`
now has centered text indicating download status and instructions

provides links to re-attempt download and practice path
Copy link
Collaborator

@camillevilla camillevilla left a comment

Choose a reason for hiding this comment

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

@PhilipDeFraties This is working well for me. Good job handling the various flavors of permissions involved and restricting attachment access appropriately. I made a small accessibility request re: "Click here" for link text. It looks like one of the practice_resource feature specs is currently failing in CI. Please also rebase in the Rails 6.1 changes from master (and Ruby 3.2.2, depending on the timing!).

@PhilipDeFraties PhilipDeFraties merged commit 32ad977 into master Nov 28, 2023
3 of 4 checks passed
@PhilipDeFraties PhilipDeFraties deleted the dm-4270-relay-resource-links branch November 28, 2023 22:39
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.

3 participants