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.

Sign up for GitHub

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

Jump to bottom

3624 - Upgrade rails #4237

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
Open

3624 - Upgrade rails #4237

wants to merge 3 commits into from

Conversation

WillNigel23
Copy link
Collaborator

Retained all specs for (with some minor changes on deprecated syntax)

@WillNigel23 WillNigel23 force-pushed the 3624-upgrade-rails-to-current-version-2 branch from 305985c to da47801 Compare July 18, 2024 22:40
WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
app/controllers/user_controller.rb
:preferred_locale,
:help,
:footer_block,
notification_attributes: [
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 can pass in accepts_nested_attributes for notification at user model so we can update them together.

Further optimization needed in update_profile logic.

WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
app/helpers/export_helper.rb
@work = Work.includes(pages: [{notes: :user}, {page_versions: :user}]).find_by(id: work.id)
render_to_string :layout => false, :template => "export/show.html.erb"
@work = Work.includes(pages: [{ notes: :user }, { page_versions: :user }]).find_by(id: work.id)
render_to_string template: 'export/show', layout: false, formats: [:html], handlers: [:erb]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This syntax seems to be not supported in newer versions of rails (I got multiple failures due to doing relative path with extention, ex: deed/deed.html, export/show.html.erb).

We instead pass in formats and handlers params

WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
app/models/ia_work.rb
@@ -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.

URI.encode is deprecated, use CGI.escape instead.

(This is also why we need to use forked version of gravatar gem, old one is not updated and is still using URI.encode)

WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
app/views/user/update_profile.html.slim
td =f.check_box("notifications[user_activity]", {checked: @user.notification.user_activity})
th =f.label :user_activity, t('.user_activity')
-if @user.owner
=f.fields_for :notification do |nf|
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 can do fields_for, this is so we don't have to manually add the names. This will fall under notification_attributes and we can pass in @user.update(user_params) and the notification will also be updated

WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
config/i18n-tasks.yml
@@ -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.

aliases in yml files are now considered unsafe and not supported anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reason: aliases in yml files are unsafe and rails moved away from supporting this

WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
config/locales/deed/deed-en.yml
@@ -6,29 +6,28 @@ en:
collection_inactive: Collection Inactive
collection_joined: Collection Joined
deed:
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.

No need to scope in .html anymore since we moved it to formats: [:html], it still looks at translations .deed instead of at .deed.html

WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
config/rubocop/.rubocop-layout.yml
@@ -0,0 +1,47 @@
Layout/EmptyLinesAroundModuleBody:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find these configs a good default startinig point for rubocop. But these are preferential so we can tweak more moving forward.

@WillNigel23 WillNigel23 force-pushed the 3624-upgrade-rails-to-current-version-2 branch 2 times, most recently from 1c048a9 to 86bc5a7 Compare July 18, 2024 22:54
WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
app/models/page.rb
after_destroy :update_featured_page, if: Proc.new {|page| page.work.featured_page == page.id}

serialize :metadata, Hash
serialize :metadata, type: Hash
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 is causing error

WillNigel23
WillNigel23 commented Jul 18, 2024
View reviewed changes
spec/models/xml_source_processor_spec.rb
context 'subject linking not disabled (default)' do
it 'builds the xml document' do
expect(work.collection).to eq(collection)
xml = page.wiki_to_xml(page, Page::TEXT_TYPE::TRANSCRIPTION)
expect(Article.all.count).to eq(3)
expect(PageArticleLink.all.count).to eq(4)
expect(xml).to eq(EXPECTED_XML)
expect(Nokogiri::XML(xml).to_s).to eq(Nokogiri::XML(EXPECTED_XML).to_s)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason eq() matching is failing due to inconsistent tabs/spaces, but contents are literally the same. Doing this ensures we compare the contents.

@WillNigel23 WillNigel23 force-pushed the 3624-upgrade-rails-to-current-version-2 branch from 41ed7b9 to 2c808ae Compare July 22, 2024 17:02
@WillNigel23 WillNigel23 force-pushed the 4225-migrate-process-document-upload-rake-to-active-job branch 3 times, most recently from e4917fc to 7d50130 Compare July 25, 2024 17:59
Base automatically changed from 4225-migrate-process-document-upload-rake-to-active-job to development July 31, 2024 15:25
@WillNigel23 WillNigel23 force-pushed the 3624-upgrade-rails-to-current-version-2 branch 2 times, most recently from 2ac7272 to 2c808ae Compare August 5, 2024 20:38
@WillNigel23 WillNigel23 force-pushed the 3624-upgrade-rails-to-current-version-2 branch 4 times, most recently from ea1ead6 to 2d3620d Compare August 5, 2024 21:21
@WillNigel23 WillNigel23 force-pushed the 3624-upgrade-rails-to-current-version-2 branch from 2d3620d to 204fff3 Compare August 5, 2024 21:28
@WillNigel23 WillNigel23 linked an issue Sep 5, 2024 that may be closed by this pull request
Upgrade Ruby and Rails to current versions #3624
Open
@WillNigel23 WillNigel23 force-pushed the 3624-upgrade-rails-to-current-version-2 branch 2 times, most recently from 6a34cf4 to 6599dd2 Compare September 5, 2024 20:13
@WillNigel23 WillNigel23 force-pushed the 3624-upgrade-rails-to-current-version-2 branch from 6599dd2 to 9457096 Compare September 5, 2024 20:19
@benwbrum benwbrum mentioned this pull request Sep 6, 2024
Closed
@bencomp bencomp mentioned this pull request Sep 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade Ruby and Rails to current versions
2 participants
@WillNigel23 @saracarl