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

fix a functional test concerning node model (#2716) #2717

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@E-L-T

E-L-T commented May 11, 2018

fixes #0000

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test

We know that tests don't pass but we are trying to fix this in this pull request

  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

Hi @publiclab/reviewers , can you check this ?

@PublicLabBot

This comment has been minimized.

PublicLabBot commented May 11, 2018

2 Messages
📖 @E-L-T Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@jywarren

This comment has been minimized.

Contributor

jywarren commented May 11, 2018

Hi, was this test giving you trouble locally?

@E-L-T

This comment has been minimized.

E-L-T commented May 11, 2018

Hi @jywarren ,
yes i had a problem with this test locally. Actually, when i launched the tests locally, i had 9 errors (I opened an issue : #2716)
After rewriting some code, I got only 1 failure and 4 errors.
It is surprising that the Travis CI build passed anyway, isn't it ?

@E-L-T E-L-T changed the title from fix a fonctionnal test concerning node model (#2716) to WIP-fix a fonctionnal test concerning node model (#2716) May 11, 2018

@E-L-T E-L-T changed the title from WIP-fix a fonctionnal test concerning node model (#2716) to [WIP]-fix a fonctionnal test concerning node model (#2716) May 11, 2018

@E-L-T E-L-T changed the title from [WIP]-fix a fonctionnal test concerning node model (#2716) to [WIP]-fix a functional test concerning node model (#2716) May 11, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented May 24, 2018

Sorry i was away traveling and got behind on this!

It is surprising! But local tests run on sqlite (to make installation easier) and Travis tests on MySQL -- we should do better to standardize or be sure both pass. I'll merge this so we get a bit closer, thank you so much! We'd love some help getting all to pass.

@@ -43,9 +43,7 @@ def self.search(query, order = :default)
.order(orderParam)
end
else
nodes = Node.limit(limit)
.where('title LIKE ?', '%' + input + '%', status: 1)

This comment has been minimized.

@jywarren

jywarren May 24, 2018

Contributor

Ah, but we do need this extra title matching -- otherwise we won't match the title against search input. This is just backup code if MySQL/MariaDB are not installed, but I still want to be sure it works.

I think the issue here is we never defined the limit parameter! Not sure how that happened, but if you re-add the original code but remove the .limit(limit) we should be OK here. Thanks so much for looking into this!!!

@E-L-T E-L-T changed the title from [WIP]-fix a functional test concerning node model (#2716) to fix a functional test concerning node model (#2716) Jun 8, 2018

@E-L-T

This comment has been minimized.

E-L-T commented Jun 8, 2018

Hey, it seems that all the tests pass locally now. Good job !
So you can close this pull request (and the corresponding issue: #2716 )?

@jywarren

This comment has been minimized.

Contributor

jywarren commented Jun 11, 2018

Oh really! Gosh, i wonder what it was. Maybe now on Rails 5? I'm happy to close this but would love a bit more info if you have it! Thanks!

@E-L-T

This comment has been minimized.

E-L-T commented Jun 13, 2018

Yes, I think the changes have been made during the rails 5.0 upgrade.
It concerns the search method of node model that was mising parameters. Method which is called in app/services/typeahead_service.rb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment