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

Deprecate Drupal legacy database structures/naming #956

Open
14 of 16 tasks
jywarren opened this issue Nov 4, 2016 · 51 comments · Fixed by #11308
Open
14 of 16 tasks

Deprecate Drupal legacy database structures/naming #956

jywarren opened this issue Nov 4, 2016 · 51 comments · Fixed by #11308
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration deprecation hall-of-fame help wanted requires help by anyone willing to contribute Ruby

Comments

@jywarren
Copy link
Member

jywarren commented Nov 4, 2016

This used to be a Drupal site (back in https://github.com/jywarren/plots days!)

This is a complex, multi-part issue which should be broken up into smaller issues, but just to provide an overview:

The legacy Drupal stuff is almost eliminated, but is confusing for new contributors -- mostly at this point it's:

The word Drupal on some models

We could rename DrupalNode to Node, for example, and this line in the model would then be unnecessary:

https://github.com/publiclab/plots2/blob/master/app/models/drupal_node.rb#L20-L21

But this would need to be a find/replace throughout the codebase!

The filename here: /app/models/drupal_node.rb could then become /app/models/node.rb

So, in summary, we've completed:

Remaining to refactor

There are a few others, but we should look into how heavily they're even used, as we could start a new project to begin simplifying/deprecating older models:

  • Code related to Answer model (not Drupal but we do want to deprecate it): https://github.com/publiclab/plots2/search?q=answer
  • drupal_content_field_image_gallery.rb - these could be merged into (added before) the body content field of the nodes they're attached to and this "gallery" feature finally dumped (broken out into Refactor legacy gallery code into node revision body #4074)
  • drupal_content_field_map_editor.rb - this and 2 below could be merged into the map node body field and not stored as separate records
  • drupal_main_image.rb - currently switching between this and a new one on these lines but could migrate old ones forward
  • drupal_file.rb - we could (with drupal_upload, below) just append files to the content of each node (which would use the Image model, actually) and get rid of these. Actually so I think we can just take the URL of the file, and add a new section to the bottom of the node, called ### Files and just make a list of the URLs to the files. I think this should work for most or all of these? Then we can delete the records -- the uploaded files themselves will remain!
  • drupal_upload.rb - this last one has no records, as far as I can tell... but links node to drupal_file so we should be able to get rid of this model and table at the same time as drupal_file.rb
@jywarren jywarren added help wanted requires help by anyone willing to contribute Ruby labels Nov 4, 2016
@jywarren jywarren mentioned this issue Nov 4, 2016
6 tasks
@jywarren jywarren added the break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration label Nov 4, 2016
@aayushgupta1
Copy link
Contributor

@jywarren I would like to work on this as no one is currently working on this. Could you just explain a bit more?

@jywarren
Copy link
Member Author

jywarren commented Nov 8, 2016

This is a pretty big and complex one, but I think you could try doing a
piece of it. For example, changing all instances of "DrupalComment" to
"Comment". Do you have a good tool (text editor, command line utility) for
doing find/replace across lots of files?

You'd also want to change "drupal_comment" to "comment" where that happens
as well. And it'll be likely to fail some tests when you first do it, but
that'll be part of the process.

On the other hand there may be a number of more clearly defined projects
you could take on that could be more suitable, as this one may be
frustrating! Just a fair warning :-)

@aayushgupta1
Copy link
Contributor

Okay. Thanks @jywarren . Which text editor would you suggest? Also if I am able to change it all, will there be any failures?

@jywarren
Copy link
Member Author

jywarren commented Nov 8, 2016

If you change everything in /app and /test, I think it will be ok but I
don't know for sure.

What system are you on? On a Linux system there's some combination of "sed"
that can be used... But it's a bit obscure to use. Search for "sed replace
across files" on stack overflow?

Perhaps the Atom editor can do this?

https://atom.io/

@ghost
Copy link

ghost commented Nov 8, 2016

@jywarren Hi I can take on changing stuff to use User instead of DrupalUsers if @aayushgupta1 is just changing the other stuff. let me know if you have any tips on how to do it and then ill figure it out :)

@aayushgupta1 I highly recommend Visual Studio Code as a text editor on Windows, Mac, and Linux. Atom is a close second but I prefer Code because it is much ligher, has better git integration and js support, and often has better autocomplete.

Once you install code, install the Ruby extension as that will make working with a ruby on rails project much easier. If you work with Python, for example, you can also get an extension for that which has amazing autocomplete out of the box :)

looking forward to working with you guys on this one! :)

@jywarren
Copy link
Member Author

jywarren commented Nov 8, 2016

Wow you two -- this sounds good, and the key here is the tests -- these
namings are so thoroughly peppered throughout the codebase, that:

a) if we break things, tests should show it
b) but only if our tests are really thorough!

We've come a LONG way on tests, and now have hundreds (!) but just a heads
up that this is an ambitious project to take on. But I'm here for you! I
might recommend that you take this on one at a time, however, because if
you edit the same files, you'll likely encounter some merge conflicts.
These aren't impossible to get around by any means, but again, just watch
out.

Much admiration for your interest in this! Thanks, and just take it one
step at a time!

@aayushgupta1
Copy link
Contributor

@ashleypt Sure. I will take up the drupal_node to node part. @jywarren can you check out line 291. If I remove drupal from there, what do I insert?

@aayushgupta1
Copy link
Contributor

@jywarren It's a great comfort to know that we have you support and look forward to work with you and @ashleypt .

@jywarren
Copy link
Member Author

jywarren commented Nov 8, 2016

Oops, line 291of what file? I'm on my phone so could you link directly?
Thanks! 🛬

@aayushgupta1
Copy link
Contributor

@jywarren
Copy link
Member Author

jywarren commented Nov 8, 2016

@ashleypt perhaps you should try drupalcomments or drupaltags first, as DrupalUsers is extra complex and let's get the easier ones first to ensure this whole process works!

@jywarren
Copy link
Member Author

jywarren commented Nov 8, 2016

Ah you can leave that, it's a method parameter, not a column or model name. Good question!

@jywarren
Copy link
Member Author

jywarren commented Nov 8, 2016

All right though I have to sign off for a while so good luck and I'll check in soon.

@aayushgupta1
Copy link
Contributor

@jywarren I changed it. But it's giving an error because of some rake db:setup. Shall I forward you the code so that you can update it.

@aayushgupta1
Copy link
Contributor

@ashleypt Is yours working?

@jywarren
Copy link
Member Author

jywarren commented Nov 8, 2016

The best place for me to look over your changes is in the PR -- thanks!

@aayushgupta1
Copy link
Contributor

Okay I'll raise that request again @jywarren

@aayushgupta1
Copy link
Contributor

@jywarren PR request #972

@sandyvern
Copy link
Collaborator

sandyvern commented Nov 8, 2016

@jywarren If you need an extra person to work on other aspects of this like drupalcomments or drupaltags I would be happy to jump in.

@ghost
Copy link

ghost commented Nov 8, 2016

@jywarren ok i guess ill take drupalcomments since my first bug was fixing a comment form :P

@sandyvern so you can work on tags I guess?

@aayushgupta1
Copy link
Contributor

aayushgupta1 commented Nov 9, 2016

@jywarren then I'll work on nodes

@jywarren
Copy link
Member Author

jywarren commented Nov 9, 2016

Thank you friends, I'm sorry, there's a lot going on at the moment, and our
annual conference is beginning. But I'll do my best to find a moment to
review and provide feedback. Thanks.

@ghost
Copy link

ghost commented Nov 10, 2016

@aayushgupta1 mentioned since you're on nodes
@jywarren so far I've changed everything but there's a problem with node_shared.rb where you define a method "comments". Ordinarily it just does self.drupal_comments and there's no infinite recursion, but it causes infinite recursion when we rename the model to comments. see this line of code:

def comments(direction = 'DESC')

in my refactored branch: https://github.com/ashleypt/plots2/blob/fix/change-drupal-comments-to-comment/app/models/concerns/node_shared.rb#L12

Related stackoverflow post about recursive virtual attributes http://stackoverflow.com/a/5446209

here's the PR I have up, in progress: #978

@jywarren
Copy link
Member Author

Ahhh, great find and good troubleshooting to figure out the reason. I'm thinking:

.order("timestamp #{direction}") is the only additional thing the comments method does in node_shared.rb

So, we could potentially delete the entire method and just let calls to comments go directly to the native comments instead of the custom method we'd written. But we'd need to search for any usage of thing.comments through the codebase and add .order("timestamp DESC") or `.order("timestamp") to those usages (since it won't be added automatically anymore) -- and there aren't that many:

(trusty)warren@localhost:~/sites/plots2$ grep -r '.comments(' app/
app/assets/javascripts/comment_expand.js:function expand_comments(question_id){
app/views/questions/show.html.erb:    <% @node.comments('ASC').each do |comment| %>
app/views/questions/show.html.erb:          expand_comments(0);
app/views/questions/_answer.html.erb:          expand_comments(<%= answer.id %>);
app/views/users/profile.html.erb:    profile.display_comments();
app/models/search_record.rb:      @nodes += SearchService.new.find_comments(input, 25) if params[:comments] || all
app/models/concerns/node_shared.rb:  def comments(direction = 'DESC')
app/services/typeahead_service.rb:  def comments(params, limit)
app/services/typeahead_service.rb:    @comments ||= find_comments(params)
app/services/typeahead_service.rb:  def find_comments(input, limit=5)
app/services/search_service.rb:    @comments ||= find_comments(params)
app/services/search_service.rb:  def find_comments(input, limit=5)

@jywarren
Copy link
Member Author

I broke out the DrupalUser task into its own issue!

#2838

Getting so close now! Thanks, all!

@jywarren
Copy link
Member Author

Broke out the Map related work into #4072 !!

@TildaDares
Copy link
Member

TildaDares commented Jul 26, 2021

@jywarren I'm a bit confused about what I'm supposed to do. Could you explain it to me?

@jywarren
Copy link
Member Author

Hi Tilda! Thanks! Basically we have all this old code which is either not used as all, or it could be refactored to use more standard and recently written code. For example, the Drupal file and image models could be modified to use the native Ruby Image model and we could get rid of old code that was written around the old Drupal database.

Part is refactoring code to stop using Drupal models. Part is adapting new code like Nodes to be able to display maps without the old code. Finally there's work to actually convert old database data to new models, using Rails migrations. All together, these will help out disconnect old code and then, once it's no longer integrated, delete it. This simplifies our code and makes it easier to maintain. Does that make sense? What else can I help explain? Thanks!!

@TildaDares
Copy link
Member

@jywarren For this first one, what should I do with the Answer model?

@jywarren
Copy link
Member Author

Whoops, sorry for the slow reply! With regard to the Answer model, all Answers are now converted into comments, so we can go through any code that uses the Answer model and remove it piece by piece. There should be no Answer records, either! They're all comments now.

@anirudhprabhakaran3
Copy link
Member

Hello! Can I try looking into this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
break-me-up break up for cleaner code separation, discrete tests, and, easier and iterative collaboration deprecation hall-of-fame help wanted requires help by anyone willing to contribute Ruby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants