search

publiclab / plots2

a collaborative knowledge-exchange platform in Rails; we welcome first-time contributors! :balloon:
https://publiclab.org
GNU General Public License v3.0
957 stars 1.83k forks source link

Deprecate Drupal legacy database structures/naming #956

Open jywarren opened 7 years ago

jywarren commented 7 years ago

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:

aayushgupta1 commented 7 years ago

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

jywarren commented 7 years ago

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 commented 7 years ago

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

jywarren commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

jywarren commented 7 years ago

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

aayushgupta1 commented 7 years ago

@jywarren https://github.com/publiclab/plots2/blob/master/app/models/drupal_node.rb#L291

jywarren commented 7 years ago

@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 commented 7 years ago

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

jywarren commented 7 years ago

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

aayushgupta1 commented 7 years ago

@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 commented 7 years ago

@ashleypt Is yours working?

jywarren commented 7 years ago

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

aayushgupta1 commented 7 years ago

Okay I'll raise that request again @jywarren

aayushgupta1 commented 7 years ago

@jywarren PR request #972

sandyvern commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@jywarren then I'll work on nodes

jywarren commented 7 years ago

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 commented 7 years ago

@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:

https://github.com/publiclab/plots2/blob/72e4820017a4ec485afbd7572011745e1177a2f3/app/models/concerns/node_shared.rb#L12

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: https://github.com/publiclab/plots2/pull/978

jywarren commented 7 years ago

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 commented 7 years ago

Actually, looking more closely through the above, the only line which is actually using that method is:

app/views/questions/show.html.erb:    <% @node.comments('ASC').each do |comment| %>
jywarren commented 7 years ago

You're really close then, great! Keep up the good work and apologies again for my slow response -- the Barnraising (our annual conference) just ended and I'm finally catching up. Normally i'll be responding much faster to issues and I appreciate your patience.

jywarren commented 7 years ago

How are folks doing on these issues? Hope all's well and I'm happy to make a suggestion of next steps if it's helpful!

jywarren commented 7 years ago

Sounds like @ashleypt is making progress, as updated in #978. Thanks!

ghost commented 7 years ago

Hey guys, I just about finished with my PR for refactoring DrupalComment and I wanted to give what I did and some issues I encountered in case that might help!

@aayushgupta1 @sandyvern

first of all keep in mind the rails naming conventions: a model is non-plural and capitalized in camel case like this: Comment, or DrupalComment. For instances of those models, we have all lowercase names, for example comment or comments. So I had to search through for DrupalComment, and refactor all of those to Comment, especially the model class! I use Visual Studio Code, but you can use any other text editor like sublime text or atom. In vs code I do ctrl-shift-f and put in DrupalComment. At each step I increment by running bundle exec rake test - "oops I need to refactor drupal_comments too, not just DrupalComment"

Two big challenges I encountered were a troublesome method within node_shared.rb and a unit test that needed to be updated: answers_test.rb

First, node_shared.rb had a comments method which had previously referred to self.drupal_comments but now referred to self.comments (ie it was calling itself recursively without end!) I spotted the bug by running rake test as always, and it was something scary and overwhelming, but it was a result of the method calling itself. How did I find out? I googled the error and found it on stackoverflow (this is very common) and someone suggested in the answers this sort of problem. You can see the post linked in the PR I believe. I found the method after that but since I didn't know the correct approach to solving the problem - "it's a simple method, should I just delete it and manually add .order to places where it's called? should I refactor it out?". I decided the best way was to ask someone more experienced with the codebase - @jywarren who's been very helpful. He pointed out that it was really only being explicitly called one place - the answers#show view (answers/show.html.erb). all I had to do was replace @node.comments('ASC') with @node.comments.order("timestamp ASC").

Then, this was more subtle, since now node.comments was referring to a descending list instead of ascending (latest last instead of first) a test was failing. This REALLY held me up - I got burned out and gave up for a week until @jywarren messaged me asking if I was stuck. I really like to solve problems on my own as I'm afraid I won't learn if I just get a solution somewhere else. This is something I need to work on as I could have been done earlier and solved many other issues by now.

It turned out to be so simple - the last and toughest change I needed to make was simply changing a comments.first (before this was the latest comment) to comments.last for a test that verified descending order in answer.comments.

Lessons learned:

Google errors, don't try to figure them out on your own unless you understand them. DO try to understand why they occur and explain it - you don't need to do something in a solipsistic vacuum yourself to understand it fully.

Read the code and use your editor's search function!

Test INCREMENTALLY!!11one

Don't get burned out and give up on the last mile - ask for help, use pomodoro timers, and check out the War of Art by Steven Pressfield (it's on youtube too if you search), a very short little book that helps with burnout, procrastination, and other forms of Resistance!

jywarren commented 7 years ago

Just wanted to say that @ashleypt's changes were just merged (#978), passing all tests. Thanks and looking forward to tackling the remaining parts of this!

jywarren commented 7 years ago

In particular, @aayushgupta1 - your PR looked really good, so if you wanted to resubmit just the DrupalNode part, I'd love to see it!

aayushgupta1 commented 7 years ago

@jywarren If this is still open I would like to continue working on it. I had some work so I wasn't available. @ashleypt Thanks for your suggestions, I'll try and use them while implementing it.

jywarren commented 7 years ago

Yes please! I believe you'd made a good start to the DrupalNode conversion... again, if we could tackle one at a time, it's easier to troubleshoot and to be sure it runs cleanly. Thanks!

aayushgupta1 commented 7 years ago

Okay I kind of have a very basic doubt. I made the changes from drupal_node to node. I downloaded the file and did the editing. Now how do I change it in github? @jywarren @ashleypt

jywarren commented 7 years ago

Hi, do you mean in your PR here: #972 ? If so, I think you can just open a new pull request, rather than change that one.

Perhaps this guide would help? https://publiclab.org/wiki/contributing-to-public-lab-software#A+sample+git+workflow

Thanks and we're happy to help if you run into trouble, don't be afraid to ask!

Pk13055 commented 7 years ago

@jywarren Have all the refactoring tasks been taken up?

jywarren commented 7 years ago

I believe someone's working in an active PR on DrupalNode - Node, you could see if they need help or take up their project if they don't have time?

Thanks!

On Dec 29, 2016 10:39 AM, "Pratik Kamble" notifications@github.com wrote:

@jywarren https://github.com/jywarren Have all the refactoring tasks been taken up?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plots2/issues/956#issuecomment-269647460, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ8fehZAwc_nwx1TtRTXL0PHNpkU0ks5rM9QegaJpZM4KpyyM .

jywarren commented 7 years ago

Hi, folks, we're getting really close here, with Tag, Comment, and Counter dealt with. If folks want to tackle the next one, now is a good time! I was thinking either DrupalNodeRevision => Revision or DrupalNodeCommunityTag => UserTag would be a good next step.

jywarren commented 7 years ago

Very exciting work by @aayushgupta1 knocked the top (and biggest) item off the list here. Only a few more to go, DrupalNodeCommunityTag and DrupalNodeRevision!!

jywarren commented 7 years ago

All the major ones are now complete, with the exception of DrupalUsers -- and we're slowly switching all uses of that table to User -- great!

jywarren commented 6 years ago

We could use a new issue/task for copy all old Drupal image records into new Rails Image records and delete old ones, in a Rails migration -- this would let us start deleting old Drupal image code and make our codebase much cleaner! @publiclab/reviewers -- anyone want to take a stab at this?

ryzokuken commented 6 years ago

@jywarren much needed! I might not be able to take this on all by myself, but I could help break this down for others.

jywarren commented 6 years ago

oh, awesome. I think even the image work could be done in parts!

On Mon, Apr 23, 2018 at 12:37 PM, Ujjwal Sharma notifications@github.com wrote:

@jywarren https://github.com/jywarren much needed! I might not be able to take this on all by myself, but I could help break this down for others.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plots2/issues/956#issuecomment-383640749, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ1RZSBAR49whb24hv-fuxY3oCdPWks5trgNQgaJpZM4KpyyM .

jywarren commented 6 years ago

I've just migrated all nodes of type "report" over to "notes" -- here were the IDs: [38, 41, 42, 45, 52, 53, 103, 143, 146, 166, 188, 214, 306, 417]

We can now deprecate all report code except in the legacy controller!

jywarren commented 6 years ago

I broke out the DrupalUser task into its own issue!

https://github.com/publiclab/plots2/issues/2838

Getting so close now! Thanks, all!

jywarren commented 5 years ago

Broke out the Map related work into https://github.com/publiclab/plots2/issues/4072 !!

TildaDares commented 3 years ago

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

jywarren commented 3 years ago

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 commented 3 years ago

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

jywarren commented 2 years ago

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.