Skip to content

Extracting method to ease future switching logic#5021

Closed
jeremyf wants to merge 2 commits intomainfrom
gh-4195-allow-import-url-job-to-receive-af-or-valkyrie
Closed

Extracting method to ease future switching logic#5021
jeremyf wants to merge 2 commits intomainfrom
gh-4195-allow-import-url-job-to-receive-af-or-valkyrie

Conversation

@jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Jul 13, 2021

Extracting method to ease future switching logic

bf9e13b

In discussion with @no_reply, in addressing #4195, we want to explore
the following path:

  1. As early as possible in the request cycle, create a
    Hyrax::UploadedFile for both a CarrierWave file or a file selected
    for upload by BrowseEverything.
  2. Building on step 1, we would then hope to reduce branching logic in the
    request cycle, and preserve existing class interfaces.

By making this non-breaking change, I'm orienting the code to deliver on
this aspirational goal.

Next steps would be:

  1. Update the Hyrax::WorksControllerBehavior to create
    Hyrax::UploadedFile records for the files selected from
    BrowseEverything (note CarrierWave handles the "inline" uploads).
  2. Adjust the extracted Hyrax::UploadedFile#perform_ingest_later
    method to handle files that were created from BrowseEverything.

The plan is to add a field to the UploadedFile (perhaps
url_of_file_to_ingest) that the Hyrax::WorksControllerBehavior would
populate based on user input parameters.

With that in place, we could look at backporting the solution into the
ActorStack.

Handling CarrierWave/BrowseEverything controller logic

b2f2af4

Prior to this commit, the Hyrax::WoksControllerBehavior's Valkyrie
branching logic ignored files selected via BrowseEverything. With
this change the controller normalizes(-ish) the two methods of file
upload:

  1. CarrierWave
  2. BrowseEverything

An immediate follow-up step is to update
Hyrax::UploadedFile.perform_ingest_later. In it's present
implementation, that method assumes a CarrierWave object. Adding a
switching case for UploadedFiles that have a
url_of_remote_file_for_ingest (e.g., a file selected via
BrowseEverything) instead of the assumptive file attribute (e.g., a
file uploaded via CarrierWave).

Relates to #4195

@jeremyf jeremyf force-pushed the gh-4195-allow-import-url-job-to-receive-af-or-valkyrie branch 5 times, most recently from ec3dfbc to 881638d Compare July 15, 2021 15:14
@jeremyf jeremyf requested a review from no-reply July 15, 2021 19:08
@jeremyf jeremyf force-pushed the gh-4195-allow-import-url-job-to-receive-af-or-valkyrie branch 4 times, most recently from 721006c to ef11511 Compare July 16, 2021 16:05
jeremyf added 2 commits July 17, 2021 09:44
In discussion with @no_reply, in addressing #4195, we want to explore
the following path:

1) As early as possible in the request cycle, create a
   Hyrax::UploadedFile for both a CarrierWave file or a file selected
   for upload by BrowseEverything.
2) Building on step 1, we would then hope to reduce branching logic in the
   request cycle, and preserve existing class interfaces.

By making this non-breaking change, I'm orienting the code to deliver on
this aspirational goal.

Next steps would be:

1) Update the Hyrax::WorksControllerBehavior to create
   Hyrax::UploadedFile records for the files selected from
   BrowseEverything (note CarrierWave handles the "inline" uploads).
2) Adjust the extracted `Hyrax::UploadedFile#perform_ingest_later`
   method to handle files that were created from BrowseEverything.

The plan is to add a field to the UploadedFile (perhaps
`url_of_file_to_ingest`) that the Hyrax::WorksControllerBehavior would
populate based on user input parameters.

With that in place, we could look at backporting the solution into the
ActorStack.
Prior to this commit, the Hyrax::WoksControllerBehavior's Valkyrie
branching logic ignored files selected via BrowseEverything.  With
this change the controller normalizes(-ish) the two methods of file
upload:

1) CarrierWave
2) BrowseEverything

An immediate follow-up step is to update
`Hyrax::UploadedFile.perform_ingest_later`.  In it's present
implementation, that method assumes a CarrierWave object.  Adding a
switching case for UploadedFiles that have a
`url_of_remote_file_for_ingest` (e.g., a file selected via
BrowseEverything) instead of the assumptive `file` attribute (e.g., a
file uploaded via CarrierWave).

Relates to #4195
@jeremyf jeremyf force-pushed the gh-4195-allow-import-url-job-to-receive-af-or-valkyrie branch from ef11511 to b2f2af4 Compare July 17, 2021 13:45
@jeremyf
Copy link
Contributor Author

jeremyf commented Jul 17, 2021

Writing some notes for future work. Based on the work of #5036 and of the current refactoring, there's continues to be some ghastly branching logic. When I started this, my expectation was that I could add a step between "upload file" and attach file to file set. That added step would optionally go and fetch the remote file.

For now, I'm going to ignore the actor stack (even though I refactored Hyrax::Actors::CreateWithRemoteFilesActor. The JobIoWrapper.create_with_varied_file_handling! appears to be a reasonable place to explore. Both the JobIoWrapper and the CreateWithRemoteFilesActor make use of the implemented (though misnamed) Hyrax::Actors::FileActor.

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

@stale stale bot added the stale label Jan 9, 2022
@jeremyf jeremyf closed this Feb 3, 2022
@orangewolf orangewolf deleted the gh-4195-allow-import-url-job-to-receive-af-or-valkyrie branch May 2, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant