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

Update to Ruby 2.7.2 and Rails 5.2.8.1 #1882

Merged
merged 23 commits into from
Jul 28, 2023

Conversation

asideofcode-dev
Copy link
Contributor

@asideofcode-dev asideofcode-dev commented Jul 23, 2023

What does the PR do ?

Resolves #1881

Resolves #1883

Resolves #1880

Update to Ruby 2.7.2 and Rails 5.2.8.1.

The main idea is to update the gems, run rails app:update (while keeping an eye on the upgrade documentation, then put in place the fixes necessary to make sure things are still working.

  1. Updates render text: to render plain:
  2. Update .gitignore
  3. Update jquery-ui-rails and boostrap
  4. For bootstrap, change data-x to data-bs-x e.g. data-toggle
  5. Reduce the count of some of the instances created by the seed file
  6. Docker related things to make developing easier
  7. Replace Planner::Application with Rails.application
  8. Moved from secret_token to secret_key_base
  9. Updated request specs to use params. See below
  10. For Fabricator, update after_save to after_create
  11. Use allow(ENV).to receive(:[]).and_call_original to avoid stub issues in updated rspec
  12. Take screenshots at failure time after js feature tests
  13. Added a temporary fix for webdrivers issue, see Webdrivers trying to load a Chrome version that doesn't exist titusfortner/webdrivers#247
  14. Moved from jquery_ujs to rails-ujs
  15. Fixed ajax:success and firing correct events for form submission on remote forms for invitations
  16. Namespaced toggle related CSS classes from toggle and collapsed to codebar-x
  17. Updated redirect_to :back to redirect_back fallback_location: root_path
  18. Defined ApplicationRecord and ApplicationMailer as per upgrade instructions
  19. Fixed haml doctype !!!
  20. Bullet.raise = false in tests. Maybe Bullet got better at detecting n+1 queries, I tried to fix some of them but there's quite a few. Best to scope that as a different effort. The queries don't seem to have caused issues until now.

For (9):

-      put :update, id: invitation.token, workshop_id: workshop.id, attending: "true"
+      put :update, params: { id: invitation.token, workshop_id: workshop.id, attending: "true" }

@asideofcode-dev asideofcode-dev marked this pull request as ready for review July 25, 2023 14:39
Gemfile Show resolved Hide resolved
@matyikriszta
Copy link
Contributor

@asideofcode-dev could you elaborate on point 12? Why was this added? To help debug tests?

@asideofcode-dev
Copy link
Contributor Author

asideofcode-dev commented Jul 25, 2023

@matyikriszta Yup, (12) was to help debug feature tests that use Javascript. There's a class of issues that a screenshot helps debug

@matyikriszta
Copy link
Contributor

@asideofcode-dev there's a bunch of new files in the bin folder, were those generated by rails app:update?

@asideofcode-dev
Copy link
Contributor Author

asideofcode-dev commented Jul 25, 2023 via email

@asideofcode-dev
Copy link
Contributor Author

asideofcode-dev commented Jul 25, 2023 via email

@matyikriszta
Copy link
Contributor

@asideofcode-dev can we fix the codeclimate/codefactor issues that flag line length? That shouldn't be too hard to do. The rest I'm ignoring for now.

@asideofcode-dev
Copy link
Contributor Author

@matyikriszta Yup, can do. I'll batch up the work once the review is done

@matyikriszta
Copy link
Contributor

@asideofcode-dev while testing the site in the browser I noticed a couple of places where the Bootstrap data- attribute was not updated resulting in broken functionality:

  • note modal on the admin member page cannot be closed with the close button: /planner/app/views/admin/members/_note.html.haml
  • none of the alerts can be dismissed, the close button does not work: /planner/app/views/layouts/_messages.html.haml
  • the Testimonials carousel on the homepage doesn't seem to work: /planner/app/views/members/_testimonials.html.haml

@matyikriszta
Copy link
Contributor

matyikriszta commented Jul 26, 2023

@asideofcode-dev this is a small cosmetic issue but on desktop the dropdown Menu gets "cut off" when open.
Screenshot 2023-07-26 at 11 47 35 am

Moderniz has been out of the picture since the move rom Foundation. A tag was unfortunately left over.
@asideofcode-dev
Copy link
Contributor Author

@matyikriszta Thanks for flagging those data- related issues. I should have really done this in the first instance...

➜  ~ curl https://cdn.jsdelivr.net/npm/bootstrap@5.0.2/dist/js/bootstrap.min.js | grep -o -E 'data-bs-[a-zA-Z0-9_-]+' | sort | uniq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 60089  100 60089    0     0    99k      0 --:--:-- --:--:-- --:--:--  100k
data-bs-content
data-bs-dismiss
data-bs-interval
data-bs-no-jquery
data-bs-original-title
data-bs-parent
data-bs-ride
data-bs-slide
data-bs-slide-to
data-bs-spy
data-bs-target
data-bs-toggle

Updates coming shortly

@asideofcode-dev asideofcode-dev force-pushed the bumping-versions branch 3 times, most recently from 12597ce to de70a94 Compare July 27, 2023 10:41
@matyikriszta
Copy link
Contributor

@asideofcode-dev just one more thing, according the latest Bootstrap 5 documentation the way to declare the a close button has slightly changed. The class has changed from .close to .btn-close and it not longer requires/supports a %span inside it with an aria-hidden="true" attribute on it. Can we update the close buttons? Currently the styles are broken. This is everywhere where close buttons are used (alerts, modals, etc).

Screenshot 2023-07-27 at 3 27 33 pm

Copy link
Contributor

@matyikriszta matyikriszta left a comment

Choose a reason for hiding this comment

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

Once the issue with the close button is resolved this is good to go. Once merged going to do another round of testing on staging. Thanks @asideofcode-dev 🎉

@asideofcode-dev
Copy link
Contributor Author

Thanks @matyikriszta. It really helps to have someone who knows how things ought to look.

New commits should address the remaining items

  1. Fix navigation menu alignment
  2. Fix close button

@matyikriszta matyikriszta merged commit 6c4541d into codebar:master Jul 28, 2023
1 check passed
@gnclmorais
Copy link
Contributor

Note: in order to run bin/rails s locally, I need to set OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES (like so, for example).

@biggianteye
Copy link
Contributor

Hi @asideofcode-dev. Do you know if this change was intentional?

commit 32f888ebc2567f286575ef61c10ee6429436d5a8
Author: asideofcode-dev <133222359+asideofcode-dev@users.noreply.github.com>
Date:   Sun Jul 23 13:03:23 2023 +0000

    Run rails app:update

diff --git a/docker-compose.yml b/docker-compose.yml
index e6e73e81..55c27f50 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -10,13 +10,7 @@ services:
     env_file:
       - docker-compose.env
     build: .
-    command:
-      - bundle
-      - exec
-      - rails
-      - server
-      - --binding=0.0.0.0
-      - --port=3000
+    command: tail -f /dev/null
     environment:
       DB_HOST: db
     volumes:

The consequence of this change is that anyone running ./bin/dstart as per the README will get a Docker container that starts up but does nothing. The expectation based on the instructions in the README is that the web server would be running in the container.

@asideofcode-dev
Copy link
Contributor Author

@biggianteye Yes, the change was intentional. By updating the command in docker-compose.yml to tail -f /dev/null, we allow the Docker container to remain active indefinitely without actively running the server process. This setup enables developers to manually start the server or other services within the container at their discretion, facilitating a more flexible development environment. This approach also reduces the dependency on the server process being the primary activity of the container, which can be beneficial for certain testing and development workflows.

@biggianteye
Copy link
Contributor

Thanks for that information. If that's the way you want people to work, it would be good to update the README to reflect this way of working. Currently it says this:

  1. Start the app

Run bin/dstart to start the app.

This should run a process which starts a server in a Docker container on your computer. This process won't finish - you'll know the server is ready when it stops doing anything and displays a message like this:

Rails 4.2.11 application starting in development on http://localhost:3000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants