Map page showing error #981

[500swapnil]

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

• all tests pass -- rake test:all
• code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
• pull request is descriptively named with #number reference back to original issue
• if possible, multiple commits squashed if they're smaller changes
• reviewed/confirmed/tested by another contributor or maintainer
• schema.rb.example has been updated if any database migrations were added

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays. Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.

Thanks!

[500swapnil]

How do I test locally, the changes I have made?

[jywarren]

Hi! It looks like this "by" should go inside the conditional if block, or if there's no author, it'll just say "by" and nothing after it, right? Would you mind moving "by" to the same line as the <a> tag 2 lines down?

[500swapnil]

Is it ok now?

[jywarren]

You could test your changes by making a fixture for a node of type "map" that has no node.map association -- that's actually defined on this line:

https://github.com/publiclab/plots2/blob/master/app/models/drupal_node.rb#L502

as:

def map
  self.drupal_content_type_map.first
end

I think we actually have only one fixture for maps, though we do test the show action here:

https://github.com/publiclab/plots2/blob/master/test/functional/map_controller_test.rb#L22-L28

The fixture is here: https://github.com/publiclab/plots2/blob/master/test/fixtures/node.yml#L135

So I believe your test could be a copy of the one for the map show action, but with a second map fixture that has no associated node.drupal_content_type_map

https://github.com/publiclab/plots2/blob/master/test/fixtures/content_type_map.yml

Or, since this is a pretty minor error, we could just leave it! I mean, really, why does this node not have a map?

[jywarren]

OK, i see your changes, and they look great. Thanks! Did you want to try writing the extra test or just calling this one done? I think writing a test is good practice, but this seems like an obscure case that we don't really need a test for.

[500swapnil]

I didnt quite understand how to add tests. And I meant that can I view the site in my own computer after making changes.

[jywarren]

So, you can add tests by adding a new entry or method to one of the test files, like the one here: https://github.com/publiclab/plots2/blob/master/test/functional/map_controller_test.rb#L22-L28

There's a good overview here: http://guides.rubyonrails.org/testing.html

This lets you not only test on your own computer (see the Testing section of the README of this project) but also runs the tests automatically so we can see them in the pull request here -- that way we can all see the same output -- and also, all the tests are run each time someone adds new code, so you can be sure future changes don't break the work you've done here. Make sense?

Anyhow, I'm going to merge this, as I think a test is a little too much for this one case. But consider writing tests for your code in the future, as it's a more reliable and collaborative way to share your new changes, and the expectations they should fulfill! Thanks!

[500swapnil]

Thank you! I will write tests next time. And I'm going to try testing the site locally now.
Any other issue I can help with?

[jywarren]

Ah, i just responded to another comment of yours with some good thoughts on testing as the first step. Thanks -- #754.

…

On Fri, Nov 25, 2016 at 12:03 PM, 500swapnil ***@***.***> wrote: Thank you! I will write tests next time. And I'm going to try testing the site locally now. Any other issue I can help with? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1019 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ1rEk5x9Tv7QxiydB8vvt_eFT7Wiks5rByL5gaJpZM4K7MsK> .

[500swapnil]

Thank you I will have a look. And yes that sounds mighty nice :)