-
Notifications
You must be signed in to change notification settings - Fork 0
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
Chore/more cleanup and housekeeping #322
Conversation
tsubik
commented
Jul 23, 2022
•
edited
Loading
edited
- updating gems just conservative update for now
- removing not needed gems
- fix DB queries when loading active admin application code, filter's collection should be proc if it makes db query or is dynamic
- upgrade ruby version
- low-hanging fruit of extra specs for admin panel (bringing up code coverage from 63% to 77%)
84ef0fa
to
74e82f4
Compare
I decided to remove all depredations in this PR as well. |
Or actually no, first we need to add couple more specs to bring up coverage to at least 80, 85% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise it looks good! (just some minor things) 👍
Should I also run a smoke test on the branch?
@@ -65,7 +62,7 @@ | |||
column :operator_document_created_at | |||
column :uploaded_by | |||
column :attachment do |o| | |||
o.attachment&.filename | |||
o&.document_file&.attachment&.filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was working? 😮
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it was not, the csv was failing, looks like noone needed it.
@@ -15,22 +16,34 @@ | |||
|
|||
if resource.is_a?(ActiveAdmin::Page) || resource.defined_actions.include?(:index) | |||
describe "GET index" do | |||
it "returns http success" do | |||
get :index | |||
subject { get :index } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ much better.
"attributes": {}, | ||
"errors": { | ||
"record": [ | ||
"record with 100188 id is not exists" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why the English here is so bad...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, I did not notice :D
# because of the problem on CI that ogr2 returns dates in different formats | ||
# and I'm not sure how to restrict format using that tool | ||
# in that case I would just ignore those properties by treating them as dynamic ones | ||
dynamic_attributes: %w[end_date last_updat start_date] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dynamic_attributes: %w[end_date last_updat start_date] | |
dynamic_attributes: %w[end_date last_update start_date] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is the exact name of that date exported from shapefile ;] this is just to not error on wrong format as I described in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explanation :)
expect(importer.results.as_json).to eq(JSON.parse(results)) | ||
|
||
expect(importer.results.to_json).to match_snapshot("importers/observations_importer") | ||
# expect(importer.results.as_json).to eq(JSON.parse(results)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot a comment line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, thanks
1862faf
to
7fdfd48
Compare
7fdfd48
to
02c7310
Compare