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

Rebuild dummy-app in Rails 7 #99

Merged
merged 4 commits into from
Nov 24, 2023
Merged

Conversation

Patrycja-Nowak
Copy link
Contributor

Changes

  • Rebuilt the dummy-app in Rails 7

Ticket

https://github.com/orgs/pixielabs/projects/4?pane=issue&itemId=31864306

Before submitting have you:

  • Included a link to any relevant ticket
  • Included links to any github issues
  • Prefix the title with the PR type (Feature, Bugfix, Documentation, Chore)

After submitting remember to:

  • Mark the ticket as ready for review
  • Include the PR on the ticket
  • Assign a single reviewer

@quorauk
Copy link

quorauk commented Aug 14, 2023

I noticed this PR has removed some of the tests that were in the /spec/dummy-app/spec directory.

I've put in a PR to move the specs out of the dummy app which you may want to have merged before you merge this one

#100

Copy link

@quorauk quorauk left a comment

Choose a reason for hiding this comment

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

The only important thing I noticed was that logging out doesn't work. I think this is a rails 7 change.

in spec/dummy-app/app/views/authenticated/index.html.erb

You will need to do the following to make it work

-<%= link_to 'Log out', destroy_user_session_path, method: :delete %>
+<%= button_to 'Log out', destroy_user_session_path, method: :delete %>

spec/dummy-app/test/application_system_test_case.rb Outdated Show resolved Hide resolved
@JonathanAndrews JonathanAndrews removed their request for review August 25, 2023 12:37
In Rails 7, link_to defaults to :get method when JS is disabled
That is why we are using button_to, which can be assigned :delete method
It was probably created when generating a lot of Rails 7 files/folders
Copy link
Collaborator

@JonathanAndrews JonathanAndrews left a comment

Choose a reason for hiding this comment

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

Look's good to me!
Great work @PatMiekina 🙏

@Patrycja-Nowak Patrycja-Nowak merged commit d3643ee into master Nov 24, 2023
1 check passed
@Patrycja-Nowak Patrycja-Nowak deleted the rebuild-dummy-app-rails7 branch November 24, 2023 13:23
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