Migrations to remove Drupal gallery and integrate into node revision #11371

anirudhprabhakaran3 · 2022-08-28T14:42:04Z

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/reviewers for help, in a comment below

This PR for now only makes the migration from Drupal Galleries to Node revisions. In another commit, I'll add the migration for the removal of the table itself.

CC: #11185

gitpod-io · 2022-08-28T14:42:13Z

jywarren

This looks good! Have you tested it out, did you need to test it in unstable or anything?

jywarren · 2022-08-29T15:46:16Z

Restarted the system test which was a random container crash.

github-actions · 2022-08-29T15:55:02Z

This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here:
https://github.com/publiclab/plots2/suites/8018661114/artifacts/345255232

jywarren · 2022-08-29T15:59:08Z

This is ready to merge if @anirudhprabhakaran3 gives a +1 on the testing/stable question!

anirudhprabhakaran3 · 2022-08-29T16:44:31Z

Yeah, I'll test it on unstable in a few minutes. I was also wondering if I should remove the _gallery.html.erb (and references to it) and make another commit. That way, all dependencies will be wrapped up in a single PR

jywarren · 2022-08-30T18:39:08Z

If you like, go ahead, but I think 2 PRs is fine too!

jywarren · 2022-08-30T18:39:24Z

Just let me know what you decide and we can merge this whenever you're ready. Great work!

codecov · 2022-09-04T05:04:33Z

Codecov Report

Merging #11371 (98fcb49) into main (5aa8720) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #11371      +/-   ##
==========================================
- Coverage   82.46%   82.41%   -0.06%     
==========================================
  Files          98       97       -1     
  Lines        5999     5975      -24     
==========================================
- Hits         4947     4924      -23     
+ Misses       1052     1051       -1

Impacted Files	Coverage Δ
app/controllers/notes_controller.rb	`85.66% <ø> (+0.57%)`	⬆️
app/models/node.rb	`91.20% <ø> (+0.07%)`	⬆️
app/services/execute_search.rb	`88.88% <0.00%> (-5.56%)`	⬇️
app/api/srch/search.rb	`66.66% <0.00%> (-3.85%)`	⬇️

github-actions · 2022-09-04T05:04:45Z

This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here:
https://github.com/publiclab/plots2/suites/8124298926/artifacts/351743698

codeclimate · 2022-09-04T05:13:41Z

Code Climate has analyzed commit 98fcb49 and detected 0 issues on this pull request.

View more on Code Climate.

github-actions · 2022-09-04T05:22:48Z

This pull request generated screenshots of many common pages in the running app. You should be able to download and view them here:
https://github.com/publiclab/plots2/suites/8124356653/artifacts/351755330

anirudhprabhakaran3 · 2022-09-04T05:42:08Z

Hi! I think we can review, test and merge this MR now. I think I've removed all the related dependencies now. Sorry for the delay!

jywarren · 2022-09-05T14:01:18Z

Hi @anirudhprabhakaran3 i'd like to try but i want to check on stable once we merge it. What is an example page of where on the live site we use the image gallery, so I can confirm it still displays the images after we merge this on stable?

jywarren · 2022-09-05T16:30:49Z

Let's check this page! https://publiclab.org/notes/eymund-diegel/10-26-2012/hello-kitty-watchdog-patrols-gowanus-canal-skies

jywarren · 2022-09-05T16:33:18Z

https://stable.publiclab.org/notes/eymund-diegel/10-26-2012/hello-kitty-watchdog-patrols-gowanus-canal-skies didn't work, strangely, even before stable began to build.

jywarren · 2022-09-05T16:58:20Z

Let's try:

https://publiclab.org/notes/juan/6-5-2012/fabaoutfit-vertical-kap-and-bap
https://stable.publiclab.org/notes/juan/6-5-2012/fabaoutfit-vertical-kap-and-bap

jywarren · 2022-09-05T16:59:47Z

jywarren · 2022-09-06T15:32:05Z

The error in stable is due to a migration:

== 20220828142008 RemoveDrupalGalleryFromNode: migrating ======================
rake aborted!
StandardError: An error has occurred, all later migrations canceled:

undefined method `gallery' for #<Node:0x000055cd4df241d8>

Do we need to run the migration first, then in a second PR, remove the 'gallery' method?

jywarren · 2022-09-06T15:32:48Z

app/models/node.rb

-                                  .order('field_image_gallery_fid')
-  end
-
-  def gallery


Yes, here, we are actually removing the method we need to use in the migration. We can use the definition itself to resolve this!

But we'll have to be sure it runs all together -- this PRs commit, and the new changes, and gets built in a container all together. Because it will fail if they are done in 2 different builds.

Fix for #11371

jywarren · 2022-09-06T15:37:55Z

db/migrate/20220828142008_remove_drupal_gallery_from_node.rb

+    Node.all.each do |node|
+      new_revision = node.revisions.first.dup
+      gallery_images = ""
+      node.gallery.each do |image|


OK, I changed this and directly referenced it as node&.drupal_content_field_image_gallery.each do |image|

4e80ae2

And noting the fixes @anirudhprabhakaran3 had added in this PR too: #11393

jywarren · 2022-09-06T15:43:48Z

Oh darn, that didn't work.

Caused by:
NoMethodError: undefined method `drupal_content_field_image_gallery' for #<Node:0x00005575ad031970>

jywarren · 2022-09-06T15:45:24Z

app/models/node.rb

@@ -359,20 +359,6 @@ def scraped_image
    match&.split('src="')&.last&.split('"')&.first
  end

-  # was unable to set up this relationship properly with ActiveRecord associations
-  def drupal_content_field_image_gallery
-    DrupalContentFieldImageGallery.where(nid: nid)


Ah its because that was also not a built-in method - we deleted it too! OK, will adapt to this too.

OK, here it is:

plots2/db/migrate/20220828142008_remove_drupal_gallery_from_node.rb

Lines 6 to 11 in 4efd035

gallery = DrupalContentFieldImageGallery.where(nid: node.nid)

.order('field_image_gallery_fid')

gallery.each do |image|

html = "<a target='_blank' href='#{image&.image&.path(:original)}'><img rel='tooltip' data-title='#{image&.description}' style='margin-bottom:4px;' class='rounded' src='#{image&.image&.path(:thumb)}' /></a>"

gallery_images << html unless image.nil? || image.image.nil? || image.image.path.nil?

end

2nd follow-up to #11371

jywarren · 2022-09-06T15:57:41Z

:-(

== 20220828142008 RemoveDrupalGalleryFromNode: migrating ======================
rake aborted!
StandardError: An error has occurred, all later migrations canceled:

uninitialized constant RemoveDrupalGalleryFromNode::DrupalContentFieldImageGallery
/app/db/migrate/20220828142008_remove_drupal_gallery_from_node.rb:6:in `block in change'

jywarren · 2022-09-06T15:59:54Z

I'm not sure why that didn't work. I'll check the schema and see if the following migration somehow got run, which would have eliminated that class?

jywarren · 2022-09-06T16:03:24Z

app/models/drupal_content_field_image_gallery.rb

@@ -1,27 +0,0 @@
-require 'php_serialize'
-
-class DrupalContentFieldImageGallery < ApplicationRecord


OK, so I think it didn't work because we deleted this entire file. So we either need to re-add it, or we need to do raw SQL. I think I will re-add this file so that the migration can succeed.

phew, a lot of editing on main, but fingers X this works: cb073ae

https://ci-j.publiclab.org/job/Plots-Stable/3311/console

…tion can complete follow-up to #11371

jywarren · 2022-09-06T16:34:45Z

OK, we'll need a few more files.


== 20220828142008 RemoveDrupalGalleryFromNode: migrating ======================
/app/app/models/user.rb:10: warning: already initialized constant Authlogic::Regex::LOGIN
/usr/local/bundle/gems/authlogic-4.4.2/lib/authlogic/regex.rb:48: warning: previous definition of LOGIN was here
You have selected Sha512 as your authlogic crypto provider. This algorithm
does not have any practical known attacks against it. However, there are
better choices.

Authlogic has no plans yet to deprecate this crypto provider. However,
we recommend transitioning to a more secure, adaptive hashing algorithm,
like scrypt. Adaptive algorithms are designed to slow down brute force
attacks, and over time the iteration count can be increased to make it
slower, so it remains resistant to brute-force search attacks even in
the face of increasing computation power.

Use the transition_from_crypto_providers option to make the transition
painless for your users.
rake aborted!
StandardError: An error has occurred, all later migrations canceled:

Couldn't find DrupalFile without an ID
/usr/local/bundle/gems/activerecord-5.2.8.1/lib/active_record/relation/finder_methods.rb:433:in `find_with_ids'
/usr/local/bundle/gems/activerecord-5.2.8.1/lib/active_record/relation/finder_methods.rb:69:in `find'
/usr/local/bundle/gems/activerecord-5.2.8.1/lib/active_record/querying.rb:5:in `find'
/usr/local/bundle/gems/composite_primary_keys-11.3.1/lib/composite_primary_keys/core.rb:34:in `find'
/app/app/models/drupal_content_field_image_gallery.rb:19:in `image'
/app/db/migrate/20220828142008_remove_drupal_gallery_from_node.rb:9:in `block (2 levels) in change'

#11371

jywarren · 2022-09-06T16:39:02Z

Oof, ok, so i'm just going to check for existence of all necessary fields before:

ae016fe

Didn't need to re-add more files, just more nil checks.

jywarren · 2022-09-06T16:51:14Z

Another error - this time, a validation error, which I think means we're close:

== 20220828142008 RemoveDrupalGalleryFromNode: migrating ======================
/app/app/models/user.rb:10: warning: already initialized constant Authlogic::Regex::LOGIN
/usr/local/bundle/gems/authlogic-4.4.2/lib/authlogic/regex.rb:48: warning: previous definition of LOGIN was here
You have selected Sha512 as your authlogic crypto provider. This algorithm
does not have any practical known attacks against it. However, there are
better choices.

Authlogic has no plans yet to deprecate this crypto provider. However,
we recommend transitioning to a more secure, adaptive hashing algorithm,
like scrypt. Adaptive algorithms are designed to slow down brute force
attacks, and over time the iteration count can be increased to make it
slower, so it remains resistant to brute-force search attacks even in
the face of increasing computation power.

Use the transition_from_crypto_providers option to make the transition
painless for your users.
rake aborted!
StandardError: An error has occurred, all later migrations canceled:

Validation failed: Body can't be blank
/usr/local/bundle/gems/activerecord-5.2.8.1/lib/active_record/validations.rb:80:in `raise_validation_error'
/usr/local/bundle/gems/activerecord-5.2.8.1/lib/active_record/validations.rb:52:in `save!'

jywarren · 2022-09-06T16:53:23Z

a4a4c59

jywarren · 2022-09-06T17:09:07Z

https://ci-j.publiclab.org/job/Plots-Stable/3313/console - odd, same error.

jywarren · 2022-09-06T17:09:46Z

That's odd. It's pretty clear here:

new_revision.save! unless new_revision.body.empty?

jywarren · 2022-09-06T18:20:42Z

So, I think what's happening is that the validation will not allow space characters... but a body of " " (one or more spaces) still reads as not .empty?

jywarren · 2022-09-06T18:28:37Z

Hmm, no, that doesn't seem right -- I checked the output of all 21 records where body is "" (from the total nids here), and all are just empty. Anyways I tried just changing all to <br /> instead and restarted the stable build to just see if we can get past that. We could repeat this on production.

jywarren · 2022-09-06T18:32:03Z

Strangely it didn't seem to try to run migrations: https://ci-j.publiclab.org/job/Plots-Stable/3315/console

I may need to trigger it with a new PR merge.

jywarren · 2022-09-06T18:37:04Z

No, it did build, but no, it also didn't work. Same error. I'm going to insert a log comment in the migration. If that doesn't work, I will try manually running the migration in the Rails console before doing more.

jywarren · 2022-09-06T19:11:15Z

OK, i believe I found it:

=> #<Node nid: 333, vid: 1277, type: "note", language: "", title: "National Geographic: LIDAR survey of tree canopy w...", u...
irb(main):014:0> n.latest.body
=> "\r\n"

So there, it's not empty, but it is whitespace.

OK, trying again, this time it got stuck on บอลเด็ด กีฬาประเภททีมฟุตบอล apologies, I can't read it, but we are now in territory where we're tripping validations for old nodes which never had to pass them, and it's unrelated to image galleries. I'm going to restrict the migration to not try saving unless there was something new added, so only galleries, and then if that fails I will narrow the nids we are dealing with to ONLY the range which actually have galleries, from https://gist.github.com/jywarren/3ab414921b40ef6d8b4df68679610d93

jywarren · 2022-09-06T19:58:29Z

Success! https://ci-j.publiclab.org/job/Plots-Stable/3318/console

jywarren · 2022-09-06T20:00:47Z

Error:

[d18ef52f-4de3-4fd2-9dff-12d2896cb0d4] Started GET "/notes/juan/6-5-2012/fabaoutfit-vertical-kap-and-bap" for 96.238.50.169 at 2022-09-06 19:58:39 +0000
[d18ef52f-4de3-4fd2-9dff-12d2896cb0d4] Processing by NotesController#show as HTML
[d18ef52f-4de3-4fd2-9dff-12d2896cb0d4]   Parameters: {"author"=>"juan", "date"=>"6-5-2012", "id"=>"fabaoutfit-vertical-kap-and-bap"}
[d18ef52f-4de3-4fd2-9dff-12d2896cb0d4] Completed 500 Internal Server Error in 727ms (ActiveRecord: 695.0ms)
[d18ef52f-4de3-4fd2-9dff-12d2896cb0d4]   
[d18ef52f-4de3-4fd2-9dff-12d2896cb0d4] Psych::DisallowedClass (Tried to load unspecified class: ActiveSupport::HashWithIndifferentAccess):
[d18ef52f-4de3-4fd2-9dff-12d2896cb0d4]   
[d18ef52f-4de3-4fd2-9dff-12d2896cb0d4] app/controllers/notes_controller.rb:59:in `show'

jywarren · 2022-09-06T20:08:30Z

Tracking that here: #11355 (comment)

jywarren · 2022-09-06T21:19:23Z

OK, that's resolved. Phew. Sorry @anirudhprabhakaran3 that was a really involved process but it highlights the challenges of removing infrastructure in the correct order. I'm glad we finally got that done, and apologies for making several commits to main, that is really not typical of the process we want, but I kept thinking we were just about to solve it. No harm done, and next time we should more thoroughly test the process on unstable. Thanks for your patience!

jywarren · 2022-09-06T21:23:21Z

Noting the images got added twice, i believe because I ran the migration twice for many pages. However this won't happen on production:

jywarren · 2022-09-06T21:23:52Z

Also i had to add the absolute hostnames to the page in the dev console to see them. They had all pointed back at stable.publiclab.org instead of publiclab.org, but when i changed it, they all appeared. Great!!!

Migrations to remove drupal gallery and integrate into node revision

8739164

anirudhprabhakaran3 requested a review from a team as a code owner August 28, 2022 14:42

jywarren reviewed Aug 29, 2022

View reviewed changes

anirudhprabhakaran3 added 2 commits September 4, 2022 10:24

Remove related files

7bdc99e

Merge branch 'main' into refactor_gallery_code

043a4fd

Merged main and removed db table and model file

98fcb49

anirudhprabhakaran3 requested a review from jywarren September 4, 2022 05:41

jywarren merged commit 916c1d2 into publiclab:main Sep 5, 2022

anirudhprabhakaran3 mentioned this pull request Sep 5, 2022

Added null safety to the Drupal gallery migration #11393

Merged

5 tasks

jywarren reviewed Sep 6, 2022

View reviewed changes

jywarren added a commit that referenced this pull request Sep 6, 2022

Adjust gallery references in drupal gallery migration

4e80ae2

Fix for #11371

jywarren reviewed Sep 6, 2022

View reviewed changes

jywarren added a commit that referenced this pull request Sep 6, 2022

additional fix for node image galleries

4efd035

2nd follow-up to #11371

jywarren reviewed Sep 6, 2022

View reviewed changes

jywarren added a commit that referenced this pull request Sep 6, 2022

Temporarily re-add drupal_content_field_image_gallery.rb so the migra…

cb073ae

…tion can complete follow-up to #11371

jywarren added a commit that referenced this pull request Sep 6, 2022

additional migration fix to image galleries

ae016fe

#11371

	gallery = DrupalContentFieldImageGallery.where(nid: node.nid)
	.order('field_image_gallery_fid')
	gallery.each do \|image\|
	html = "<a target='_blank' href='#{image&.image&.path(:original)}'><img rel='tooltip' data-title='#{image&.description}' style='margin-bottom:4px;' class='rounded' src='#{image&.image&.path(:thumb)}' /></a>"
	gallery_images << html unless image.nil? \|\| image.image.nil? \|\| image.image.path.nil?
	end

		@@ -1,27 +0,0 @@
		require 'php_serialize'

		class DrupalContentFieldImageGallery < ApplicationRecord

Migrations to remove Drupal gallery and integrate into node revision #11371

Migrations to remove Drupal gallery and integrate into node revision #11371

Conversation

anirudhprabhakaran3 commented Aug 28, 2022 • edited Loading

gitpod-io bot commented Aug 28, 2022

jywarren left a comment

Choose a reason for hiding this comment

jywarren commented Aug 29, 2022

github-actions bot commented Aug 29, 2022

jywarren commented Aug 29, 2022

anirudhprabhakaran3 commented Aug 29, 2022

jywarren commented Aug 30, 2022

jywarren commented Aug 30, 2022

codecov bot commented Sep 4, 2022 • edited Loading

Codecov Report

github-actions bot commented Sep 4, 2022

codeclimate bot commented Sep 4, 2022

github-actions bot commented Sep 4, 2022

anirudhprabhakaran3 commented Sep 4, 2022

jywarren commented Sep 5, 2022

jywarren commented Sep 5, 2022

jywarren commented Sep 5, 2022

jywarren commented Sep 5, 2022

jywarren commented Sep 5, 2022

jywarren commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022 • edited Loading

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

jywarren commented Sep 6, 2022

anirudhprabhakaran3 commented Aug 28, 2022 •

edited

Loading

codecov bot commented Sep 4, 2022 •

edited

Loading

jywarren commented Sep 6, 2022 •

edited

Loading