mapknitter icon indicating copy to clipboard operation
mapknitter copied to clipboard

Schematask

Open sashadev-sky opened this issue 5 years ago • 8 comments

Fixes #1006 #959 (<=== Add issue number here) Also #938 but this updates using a rake task which is at least safer than copy paste

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/mapknitter-reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

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

Thanks!

sashadev-sky avatar Sep 10 '19 23:09 sashadev-sky

Code Climate has analyzed commit 5b1f8828 and detected 0 issues on this pull request.

View more on Code Climate.

codeclimate[bot] avatar Sep 10 '19 23:09 codeclimate[bot]

Codecov Report

Merging #1007 into main will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1007   +/-   ##
=====================================
  Coverage   73.7%   73.7%           
=====================================
  Files         40      40           
  Lines       1388    1388           
=====================================
  Hits        1023    1023           
  Misses       365     365

codecov[bot] avatar Sep 10 '19 23:09 codecov[bot]

@jywarren like mentioned before, I don't think we should keep this file as there are a lot of opportunities for human error when the schema.rb is being copy-pasted and we don't always remember to update it. take #959 for example.

  1. I would delete this file in this PR instead, but if we are going to keep it we should create a way to maintain it.

  2. This updates the schema.rb.example, which is currently outdated, and solves the human error issue by making the updating with a rake task. $ rake copy_schema

If we decided to move forward with 2 instead of 1, we should merge this so they are up to date and i will follow up with a PR to make the rake task run whenever the original schema.rb is updated.

sashadev-sky avatar Sep 10 '19 23:09 sashadev-sky

not sure why the exporter test is failing now, if the last merged PR into main was mine and it wasn't failing there...

@alaxalves I think you've manaegd to fix this before?

 FAIL["test_should_export_warpable_using_isolated_exporter_lib", #<Minitest::Reporters::Suite:0x00007f96dec02598 @name="ExporterTest">, 30.35520500014536]
 test_should_export_warpable_using_isolated_exporter_lib#ExporterTest (30.36s)
        Expected false to be truthy.
        test/models/exporter_test.rb:38:in `block in <class:ExporterTest>'

@jywarren

sashadev-sky avatar Sep 11 '19 11:09 sashadev-sky

not sure why the exporter test is failing now, if the last merged PR into main was mine and it wasn't failing there...

@alaxalves I think you've manaegd to fix this before?

 FAIL["test_should_export_warpable_using_isolated_exporter_lib", #<Minitest::Reporters::Suite:0x00007f96dec02598 @name="ExporterTest">, 30.35520500014536]
 test_should_export_warpable_using_isolated_exporter_lib#ExporterTest (30.36s)
        Expected false to be truthy.
        test/models/exporter_test.rb:38:in `block in <class:ExporterTest>'

@jywarren

It makes no sense this test to be breaking. I have no idea :smile: :smile: :smile:

alaxalves avatar Sep 11 '19 11:09 alaxalves

not sure why the exporter test is failing now, if the last merged PR into main was mine and it wasn't failing there... @alaxalves I think you've manaegd to fix this before?

 FAIL["test_should_export_warpable_using_isolated_exporter_lib", #<Minitest::Reporters::Suite:0x00007f96dec02598 @name="ExporterTest">, 30.35520500014536]
 test_should_export_warpable_using_isolated_exporter_lib#ExporterTest (30.36s)
        Expected false to be truthy.
        test/models/exporter_test.rb:38:in `block in <class:ExporterTest>'

@jywarren

It makes no sense this test to be breaking. I have no idea 😄 😄 😄

It didn't fail on another PR I just pushed #1011 . But is consistently failing here locally too 🤔

sashadev-sky avatar Sep 11 '19 11:09 sashadev-sky

Very strange re: exporter test... 😕

As to schema.rb, in plots2 we have for a long time not tracked schema.rb but provided schema.rb.example, which in production on publiclab.org means that we don't get schema.rb conflicts when running migrations. But that, when setting it up in development locally, you can manually copy the schema.rb.example file to schema.rb to get the app running. It's been a pretty stable and reliable system, if you'd like we can reproduce it here?

In fact, we even have a Dangerfile line to warn people to keep schema.rb.example up to date:

https://github.com/publiclab/plots2/blob/19916745ce9f361975cdc769537f8ccf72317766/Dangerfile#L15-L17

What do you think?

jywarren avatar Sep 11 '19 14:09 jywarren

Yeah sounds good! Rails docs say to resolve mergee conflicts just run $ rake db migrate: https://edgeguides.rubyonrails.org/active_record_migrations.html#schema-dumps-and-source-control so that is another option.

How would you like to proceed here? @jywarren

sashadev-sky avatar Sep 12 '19 00:09 sashadev-sky