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

Upgrades to browse-everything 2.x-stable #3754

Merged

Conversation

jrgriffiniii
Copy link
Contributor

@jrgriffiniii jrgriffiniii commented Mar 9, 2020

This will upgrade to the first browse-everything 2.0.0 alpha pre-release. This shall include integrated support for the the React user interface (which also integrates the Google Picker widget for supporting Google Drive uploads).

This is blocked by #3752 due to its dependency on NodeJS 12.x releases.

fixes #3604
fixes #3791
fixes #3724
fixes #3591
fixes #3606
fixes #3505
fixes #3849

On merge please resolve https://app.honeybadger.io/projects/53391/faults/60157535

TODO:

upstream:

locally:

  • Upgrade to rails 5.2
  • Rename BrowseEverythingIngester
  • Fix staging database (@HackMasterA )
  • Use shared drive for activestorage files (@HackMasterA )
  • merge Introduces the browse-everything update for non-ActiveStorage file uploads and ActiveStorage cleaning #3834 (don't copy local files, clean up tmp dir)
  • CloudIngester should happen in the background. (@tpendragon)
  • QA all the tickets we think this fixes
  • remove MVW option from page (@tpendragon)
  • document required directory structure on page
  • Test for the source metadata identifier behavior (should work for finding aids components/bibids) (@tpendragon)
  • fix test coverage
  • Make spec/resources/scanned_resources/scanned_resources_controller_spec.rb not stub BrowseEverything::UploadJob, and send real files.
  • Adjust cloud ingest specs in spec/controllers/bulk_ingest_controller_spec.rb to have accurate BrowseEverything::Uploads, do not stub BrowseEverything::UploadJob, and instead stub the Google Drive provider's #find_bytestream and #find_container methods.
  • Test that BE is cleaning up files?
  • Rebase this PR
  • try deleting that new block from file_appender (ensure pending_uploads on all resources are blank post-ingest)

@jrgriffiniii
Copy link
Contributor Author

The header for the layout seems to no longer be rendering in the staging environment, but does so for me locally:

image

image

@tpendragon tpendragon temporarily deployed to staging March 10, 2020 16:14 Inactive
@jrgriffiniii
Copy link
Contributor Author

https://circleci.com/gh/pulibrary/figgy/25098#tests/containers/1 is failing due to tests which I cannot trigger failures for locally:

Failures:

  1) ArchivalMediaCollectionDecorator raw imported metadata is not displayed
     Failure/Error: h.visibility_badge(visibility, public_readable_state?)
     
     NoMethodError:
       undefined method `visibility_badge' for #<#<Class:0x0000563ebc0646a8>:0x0000563ebca191a8>
     # ./app/decorators/valkyrie/resource_decorator.rb:48:in `block in visibility'
     # ./app/decorators/valkyrie/resource_decorator.rb:47:in `map'
     # ./app/decorators/valkyrie/resource_decorator.rb:47:in `visibility'
     # ./app/decorators/application_decorator.rb:39:in `[]'
     # ./app/decorators/application_decorator.rb:63:in `block in to_h'
     # ./app/decorators/application_decorator.rb:62:in `map'
     # ./app/decorators/application_decorator.rb:62:in `to_h'
     # ./app/decorators/application_decorator.rb:45:in `display_attributes'
     # ./app/resources/collections/archival_media_collection_decorator.rb:13:in `display_attributes'
     # ./spec/resources/archival_media_collections/archival_media_collection_decorator_spec.rb:49:in `block (3 levels) in <top (required)>'

  2) ArchivalMediaCollectionDecorator#identifier displays the identifier
     Failure/Error: h.visibility_badge(visibility, public_readable_state?)
     
     NoMethodError:
       undefined method `visibility_badge' for #<#<Class:0x0000563ebc0646a8>:0x0000563ebca191a8>

@jrgriffiniii
Copy link
Contributor Author

I've updated this in order to ensure that the BrowseEverything persistence models are properly namespaced, but pending the CircleCI build passing, this should prepare for the upstream release of the Gem.

@jrgriffiniii
Copy link
Contributor Author

Unfortunately this cannot use a Gem pre-release until there is another Hyrax 2.x release (please see samvera/browse-everything#335 (comment)).

@jrgriffiniii jrgriffiniii changed the title Upgrades to browse-everything 2.0.0.pre.alpha1 Upgrades to browse-everything 2.x-stable Mar 12, 2020
@jrgriffiniii jrgriffiniii force-pushed the issues-3724-jrgriffiniii-browse-everything-2.0.0alpha1 branch 2 times, most recently from f21379d to 2ef4f55 Compare March 12, 2020 16:33
@jrgriffiniii
Copy link
Contributor Author

jrgriffiniii commented Mar 12, 2020

These commits prevent the Order Manager interface from being rendered on the staging environment, but this is not the case in my local environment.

@jrgriffiniii jrgriffiniii force-pushed the issues-3724-jrgriffiniii-browse-everything-2.0.0alpha1 branch from 2ef4f55 to 1a84027 Compare March 12, 2020 17:13
@tpendragon tpendragon temporarily deployed to staging March 12, 2020 17:21 Inactive
@jrgriffiniii
Copy link
Contributor Author

The following error is logged when JavaScript dependencies is loaded in the staging environment:

clipboard.js:660 Uncaught TypeError: o is not a function
    at Module.<anonymous> (clipboard.js:660)
    at n (clipboard.js:66)
    at clipboard.js:260
    at o (clipboard.js:14)
    at Object.<anonymous> (clipboard.js:10)
    at Object.<anonymous> (application-ef8e1694b54505ac5770.js:2)
    at n (bootstrap:19)
    at Module.<anonymous> (auth_link_clipboard.js:1)
    at n (bootstrap:19)
    at Module.<anonymous> (application-ef8e1694b54505ac5770.js:2)

Updating clipboard with f6a2059 did not resolve this.

@tpendragon tpendragon temporarily deployed to staging March 12, 2020 18:18 Inactive
@jrgriffiniii
Copy link
Contributor Author

I've been trying to isolate the JavaScript packages which might be triggering this. Thus far, I have limited these changes to require only the following:

"@babel/preset-react": "^7.8.3",
"react": "^16.12.0",
"react-dom": "^16.12.0",
"react-redux": "^7.1.3",
"redux-logger": "^3.0.6",
"babel-loader": "^8.0.6",
"webpack-cli": "^3.3.11",

redux-logger and webpack-cli are required for running a local development environment. @babel/preset-react and babel-loader are necessary for integrating React into the Rails application, and the remaining dependencies are just for React itself.

Once the staging environment is clear, I will attempt walking back through additional dependencies in order to determine which of this is triggering the error with clipboard.

@tpendragon tpendragon temporarily deployed to staging March 12, 2020 20:43 Inactive
@jrgriffiniii
Copy link
Contributor Author

Reducing these dependencies did not resolve the error.

@tpendragon tpendragon temporarily deployed to staging March 12, 2020 21:01 Inactive
@jrgriffiniii
Copy link
Contributor Author

Simply disabling the Clipboard constructor with 55f3d37 solved the error, so I suspect that this is somehow related to the syntax of the constructor itself.

@tpendragon tpendragon temporarily deployed to staging March 12, 2020 21:10 Inactive
@jrgriffiniii jrgriffiniii force-pushed the issues-3724-jrgriffiniii-browse-everything-2.0.0alpha1 branch from 13a487c to 16ec7ea Compare March 12, 2020 21:14
@tpendragon tpendragon temporarily deployed to staging March 13, 2020 00:36 Inactive
@tpendragon tpendragon temporarily deployed to staging March 13, 2020 00:59 Inactive
@jrgriffiniii
Copy link
Contributor Author

The minified release of Clipboard does not break for my local environment, but it does on staging. When I use a CDN for the source (e. g. https://cdn.jsdelivr.net/npm/[email protected]/dist/clipboard.js) it works.

@tpendragon tpendragon temporarily deployed to staging March 13, 2020 01:13 Inactive
@tpendragon tpendragon temporarily deployed to staging March 13, 2020 01:21 Inactive
@tpendragon tpendragon temporarily deployed to staging March 13, 2020 01:37 Inactive
@jrgriffiniii
Copy link
Contributor Author

Adding <script src="//cdn.jsdelivr.net/npm/[email protected]/dist/clipboard.js"></script> to the app/views/layouts/application.html.erb view partial seems to be the only way I can find to resolve this. Explicitly importing from the source using import ClipboardJS from '../../../node_modules/clipboard/dist/clipboard.js' simply raises the same error (I suspect that this somehow relates to a Webpack configuration setting used to transpile the ES6 from the clipboard dependency).

@jrgriffiniii jrgriffiniii force-pushed the issues-3724-jrgriffiniii-browse-everything-2.0.0alpha1 branch from 3f20641 to b9f84c7 Compare March 13, 2020 01:58
@tpendragon tpendragon temporarily deployed to staging March 13, 2020 02:03 Inactive
@jrgriffiniii jrgriffiniii marked this pull request as ready for review March 13, 2020 02:12
@tpendragon tpendragon force-pushed the issues-3724-jrgriffiniii-browse-everything-2.0.0alpha1 branch from 7f9c3f2 to 19f1ac2 Compare May 5, 2020 22:48
tpendragon
tpendragon previously approved these changes May 6, 2020
@@ -0,0 +1,68 @@
# BrowseEverything::Provider which is powered off a simple hash. Useful for
# testing cloud functionality without hitting the cloud.
class HashProvider
Copy link
Member

Choose a reason for hiding this comment

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

😍

@eliotjordan eliotjordan self-requested a review May 6, 2020 17:43
Copy link
Member

@eliotjordan eliotjordan left a comment

Choose a reason for hiding this comment

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

I have a few comments, but overall this looks great. It's quite a bit a of work to get this all wired up. 👏

Also, I'm trying out a comment labeling process I read about recently:
https://conventionalcomments.org/

def upload_files(upload_id)
# Ensure files are downloaded via ActiveStorage. We delay this for
# performance.
BrowseEverything::UploadJob.perform_now(upload_id: upload_id)
Copy link
Member

Choose a reason for hiding this comment

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

question: Comment above says this is delayed for performance, however this job is performed synchronously (perform_now). Am I misinterpreting this, or is the comment incorrect?

class Upload
def perform_job; end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: This overrides directory seems to be a significant piece of new architecture. I've done a fair bit of monkey patching with other projects and this is a good reference:
https://www.justinweiss.com/articles/3-ways-to-monkey-patch-without-making-a-mess/

What about creating a new file in app/overrides/browse_everything/upload/override.rb:

module BrowseEverything
  module Upload
    module Override
      def perform_job; end
    end
  end
end

BrowseEverything::Upload.include BrowseEverything::Upload::Override

Alternatively, I've seen monkey patches in config/initializers/.

question: Are there other monkey patches in Figgy that we should move to this directory?

Copy link
Contributor

Choose a reason for hiding this comment

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

include won't work, but I can use prepend.

end
end

class BrowseEverythingLocalIngester
Copy link
Member

@eliotjordan eliotjordan May 6, 2020

Choose a reason for hiding this comment

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

thought: When I read through the controller code, I was expecting to find BulkCloudIngester in the same file as BrowseEverythingLocalIngester, but it's been extracted into a separate class. I wonder if BrowseEverythingLocalIngester should be extracted as well?

@tpendragon tpendragon merged commit 3889954 into master May 7, 2020
@tpendragon tpendragon deleted the issues-3724-jrgriffiniii-browse-everything-2.0.0alpha1 branch May 7, 2020 18:05
hackartisan added a commit to pulibrary/princeton_ansible that referenced this pull request May 7, 2020
Configure ACTIVE_STORAGE_ROOT for figgy
kelynch pushed a commit to pulibrary/princeton_ansible that referenced this pull request Jun 23, 2020
Configure ACTIVE_STORAGE_ROOT for figgy
kelynch pushed a commit to pulibrary/princeton_ansible that referenced this pull request Jun 23, 2020
Configure ACTIVE_STORAGE_ROOT for figgy
kelynch pushed a commit to pulibrary/princeton_ansible that referenced this pull request Jun 23, 2020
Configure ACTIVE_STORAGE_ROOT for figgy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment