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

3624 - Upgrade #4429

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

3624 - Upgrade #4429

wants to merge 1 commit into from

Conversation

WillNigel23
Copy link
Collaborator

No description provided.

@WillNigel23 WillNigel23 force-pushed the 3624-upgrade branch 7 times, most recently from eda90ee to 5272089 Compare November 28, 2024 02:29
@WillNigel23
Copy link
Collaborator Author

WillNigel23 commented Nov 28, 2024

Some notable changes:

Ruby Version: 3.3.5

Rails Version 7.2.1

  1. .to_s(:db) is deprecated. We now use .to_fs(:db) for formatting time string that is db compatible.
  2. File.exists? and Dir.exists? methods are both now just .exist?
  3. render syntax render 'namespace/partial.html is now not supported. We should remove the .html and instead pass it as formats: [:html]. Take a look at notes/note.html's past implementation and compare to now.
  4. For handling .dot files like graph/graph.dot, we introduced new template handler for :dot files. Like Refactor bulk image import to use Rake instead of BackgrounDRb #3, we can pass it thru formats: [:dot]
  5. I refactored interactors to inherit from ApplicationInteractor so that it follows a higher class architecture.
  6. Some capybara methods are failing after update (due to not waiting for elements to fully load before accessing in tests). Adjusted these in legacy specs to use capybara methods that have built in wait implementation to avoid unnecessary timing errors.

TODO (likely on a future ticket)

  1. Add hotwire turbo and move away from handling ajax requests server side
  2. Migrate old js files to use importmaps syntax

gem 'iiif-presentation', git: 'https://github.com/benwbrum/osullivan', branch: 'service_is_array'
gem 'riiif', git: 'https://github.com/BrumfieldLabs/riiif.git', branch: 'quote-paths-for-shell'
gem 'iiif-presentation'
gem 'riiif'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checked these forks and compared with main, it seems the changes applied here in forks are done already in main (merged from another PR I guess).

So we should be good to get these from main repo.

@@ -75,10 +78,6 @@ gem 'terser'

gem 'interactor-rails', '~> 2.0'

group :assets do
gem 'uglifier'
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not used anymore

@@ -119,7 +118,7 @@ gem 'autoprefixer-rails'
gem 'slim'

# Gravatar Image Tag
gem 'gravatar_image_tag'
gem 'gravatar_image_tag', github: 'Tinix/gravatar_image_tag'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Original gravatar_image_tag gem is not updated anymore. It contains deprecated functions like URI.encode.

This fork addressed that. Though eventually we may want to move away totally from this (as I think gravatar_image is just an API after all so we maybe don't need our own gem for this)

link_max: link_max,
min_rank: min_rank
},
formats: [:dot]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We added new mime_type :dot, which is handled like erb (technically how it is being handled before, but the updated syntax fails so we needed to add this)

@@ -32,12 +32,12 @@ def list

# Scope for date
if params[:start_date]
start_date = params[:start_date].to_datetime.to_s(:db)
start_date = params[:start_date].to_datetime.to_fs(:db)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to_s(:db) is deprecated, we now use to_fs(:db)

@@ -95,7 +94,10 @@ def update

redirect_to collection_facets_path(collection.owner, collection), notice: t('collection.facets.collection_facets_updated_successfully')
else
render('collection/facets', :locals => { :@metadata_coverages => collection.metadata_coverages, :@errors => errors })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

global variables now can't be passed thru locals. We should just declare the globals here then

@@ -28,7 +28,7 @@ def create
if @note.save
record_deed
render json: {
html: render_to_string(partial: 'note.html', locals: { note: @note }, formats: [:html]),
html: render_to_string(partial: 'note', locals: { note: @note }, formats: [:html]),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We pass in formats: [:html] instead of 'note.html' as that syntax is not supported anymore.


belongs_to :visit, optional: true
belongs_to :user, optional: true

serialize :properties, JSON
serialize :properties, coder: JSON
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New syntax. Old one is failing.

new: 'new',
queued: 'queued',
processing: 'processing',
finished: 'finished',
error: 'error'
}, _prefix: :status
}, prefix: :status
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New enum syntax

unless File.exists?(cache_file_path)
unless Dir.exists?(cache_dir_path)
unless File.exist?(cache_file_path)
unless Dir.exist?(cache_dir_path)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Function was renamed

@@ -165,7 +165,7 @@ def ingest_work(id)

self.save!
# now fetch the scandata.xml file and parse it
scandata_url = "http://#{server}#{dir}/#{URI.encode(scandata_file)}" # will not work on new format: we cannot assume filenames are re-named with their content
scandata_url = "http://#{server}#{dir}/#{CGI.escape(scandata_file)}" # will not work on new format: we cannot assume filenames are re-named with their content
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deprecated function URI.encode is replaced by CGI.escape

@@ -106,16 +106,34 @@ ignore_missing:
- 'deed.deed.*'
- 'article.graph.title'
- 'thredded.*'
es: &NOT_EN
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not supported anymore. This seems to be a security vulnerability that i18n have patched long ago.

@@ -2,3 +2,6 @@

# Add new mime types for use in respond_to blocks:
# Mime::Type.register "text/richtext", :rtf

Mime::Type.register 'text/vnd.graphviz', :dot
ActionView::Template.register_template_handler :erb, ActionView::Template::Handlers::ERB.new
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We register :dot handler

@WillNigel23 WillNigel23 force-pushed the 3624-upgrade branch 4 times, most recently from 457c943 to 7eb7964 Compare December 2, 2024 17:49
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.

1 participant