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

Ruby3 #3518

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Ruby3 #3518

wants to merge 41 commits into from

Conversation

anitagraham
Copy link
Contributor

This PR (and its matching PR on https://github.com/refinery/refinerycms-i18n ) allow refinerycms to run with Ruby 3.

It has been tested with Ruby 3.1.2 and Rails 6.1.6.1
Will shortly be tested on Rails 7.

One test failure on /resources, which I haven't been able to fathom.

Ruby 3.1.2
core: TranslationHelper, SiteBarHelper - using Ruby3 keyword args
      lib/refinery/engine uses new mobility configuration
images, resources: Updated examples to pass an explicit block to 'expect' as passing an implicit block is deprecated.
images: Use errors.add to add errors to an object - had already been done for resources.

Gem versions changed:
  mobility 1.2.9
  rspec-rails 6.0.0.rc1
  friendly_id >= 5.4.0
  friendly_id-mobility ~> 1.0.3
@anitagraham
Copy link
Contributor Author

Tests failing:

I have seen some of these errors during testing, but thought I was past them. Many seem to be timeouts, and then the reporting of timeouts seems to be a problem too.

There are many variations of:

 1.1) Failure/Error: **visit refinery.admin_images_path**
          RuntimeError:
            Rack application timed out during boot
          # ./core/spec/system/refinery/admin/xhr_paging_spec.rb:19:in `block (3 levels) in <module:Refinery>'

and I saw some other reporting of timeouts on visit, as a result of changes to bundler, I think.

Also frequently seen (below), which seems to be an error reporting timeouts. It occurs every minute during a run.

{"time":"2022-07-18T05:05:32+00:00",
 "severity":"warn",
 "class":"Async::Task",
 "oid":130980,
 "pid":2890,
 "subject":
    "#<Async::Task:0x000000000001ffa4 /home/runner/work/refinerycms/refinerycms/vendor/bundle/ruby/3.1.0/gems/async-2.0.3/lib/async/task.rb:82:in
    `backtrace' (failed)>",
    "message":[
      "Task may have ended with unhandled exception.",
      "undefined method `logger' for Async:Module
					Async.logger.debug (self) {\"Running server...\"}
					     ^^^^^^^"
					     ],
		"error":{
		  "kind":"NoMethodError",
		  "message":"undefined method `logger' for Async:Module
					Async.logger.debug (self) {\"Running server...\"}
					     ^^^^^^^",
     "stack":
      "/home/runner/work/refinerycms/refinerycms/vendor/bundle/ruby/3.1.0/gems/falcon-capybara-1.5.1/lib/falcon/capybara/wrapper.rb:61:in `block in call'
        /home/runner/work/refinerycms/refinerycms/vendor/bundle/ruby/3.1.0/gems/async-2.0.3/lib/async/task.rb:255:in `block in schedule'
    "}
}

Tidy up configuration block for Mobility 1.2.9
Revert some changes for single argument keyword args
Add descriptions for Generator spec matchers (thereby avoiding a very long message when there are no descriptions).
@parndt
Copy link
Member

parndt commented Nov 14, 2022

Thank you for looking at this

@pdornfel
Copy link

Is there any update on the status of this PR? I need to upgrade to ruby 3.2.1 and rails 6.x because I need to upgrade to the newest cflinux4 buildpack and no previous versions of ruby are supported. I currently use refinerycms with ruby 2.5.5 and rails 5.x.

I think this merge will help me upgrade to the versions of ruby and rails mentioned above. Thank you.

@@ -97,3 +97,4 @@ Gemfile.lock

# rspec failures
.rspec_failures
Refinery Review.md
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Refinery Review.md

@Matho Matho mentioned this pull request May 19, 2023
@parndt
Copy link
Member

parndt commented May 22, 2023

@pdornfel I'd really love to merge this but CI isn't happy. The corresponding PR for refinerycms-i18n is failing on Ruby 3.0 and 3.2, but not 3.1 🤔 refinery/refinerycms-i18n#104

Any help you or anyone else can lend with figuring out why this is happening would be greatly appreciated. Right now, I suspect the routing-filter gem we use.

…isting file spec/support/capybara.rb.

remove requirement for 'webdrivers/chromedriver' as this is all handled by Selenium

consistent errors in Selenium chrome webdriver, temporarily switched to firefox.
…to target the part of the url which should change.
…was failing in Refinery::Admin tests, so now using Refinery::Pages(*)
…e options hash, as options[:original_options]
return ImageObjects from Refinery::Admin::ImagesController#index action
unify code for grid_view and list_view into _image.html.erb
add view_objects directory to engine autoload_path
 change 0px to unitless 0.
extend image mixins
use mixins to handle grid view and list view for images.
extend image mixins
use mixins to handle grid view and list view for images.
replace hash.merge with Ruby3 keyword format
…id_view'.

(view, format and layout are already special words, and I may yet change format to something useful, but not duplicating a term used in another context)

keep the format variable as a symbol wherever it is used.

renamed configuration accessors to 'index_formats' (et al) to be clear about their function.
simplify view code for the admin index of a page.

put selectors used in testing into single file (still some way to go)
move helpers for actions (edit/preview/delete etc) from tag_helper.rb to (new)action_helper.rb
move helpers for actions (edit/preview/delete etc) from tag_helper.rb to (new)action_helper.rb

simplify view code for the admin index of a page.

put selectors used in testing into single file (still some way to go)
use simple selectors for file_type icons, use IconHelper to match file_type to icon

use grid layout for images grid
use data-attributes hash in markup, in place of "data-#{attributes}..."

define image_presenters for admin/index
unify code for grid_view and list_view

presenters:
group_presenter:  handle the formatting for a group of images

list_presenter and grid_presenter inherit from group presenter,

image_presenter:  generate an entry for the image_index, including clickable links, text information, and actions.

configuration:

define admin_image_sizes in configuration

 replace hard coded image sizes from several locations in views and helpers.

rename Refinery::Images.image_views to Refinery::Images.index_views

// note: looking for a better name for these I tried :formats and :layouts before noting that they, like :views, are magic words for Ruby/Rails and are, therefore, no better. //

remove temporary changes to admin/images_controller
remove image view objects (using image presenters)

split grid_view and list_view in the test contexts
add new helper 'locales_with_translated_field'  in TranslationHelper to replace repeated code block in index templates.

add test for helper

call from index pages to identify locales with existing translations
add new helper 'edit_in_locales' to ActionHelper

call from index pages to offer links to records with translations
add edit_in_locales to images/pages/resources indexes
add newline at eof for resources/admin/form
… and 'grid_view'. (view, format and layout are already special words, and I may yet change format to something useful, but not duplicating a term used in another context)"

This reverts commit 6eafef5
define colours for locale icon backgrounds and text

use id attribute of link to determine id

define mixins for locale elements
remove browser-specific selectors from the rounded-corner mixins
share code generating a link to switch/set locale

change name from 'locales_with_titles' to 'locales_with_translated_field' to better reflect what it does.

add messages keys for language and locale in en.yml
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