Skip to content
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

Merged
merged 15 commits into from
Aug 1, 2022
Merged

Chore/more cleanup and housekeeping #322

merged 15 commits into from
Aug 1, 2022

Conversation

tsubik
Copy link
Collaborator

@tsubik tsubik commented Jul 23, 2022

  • 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%)

@tsubik tsubik requested a review from santostiago July 23, 2022 14:51
@tsubik tsubik changed the title Chore/more cleanup Chore/more cleanup and housekeeping Jul 23, 2022
@tsubik
Copy link
Collaborator Author

tsubik commented Jul 23, 2022

I decided to remove all depredations in this PR as well.

@tsubik
Copy link
Collaborator Author

tsubik commented Jul 23, 2022

Or actually no, first we need to add couple more specs to bring up coverage to at least 80, 85%

Copy link
Contributor

@santostiago santostiago left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was working? 😮

Copy link
Collaborator Author

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 }
Copy link
Contributor

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"
Copy link
Contributor

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...

Copy link
Collaborator Author

@tsubik tsubik Jul 28, 2022

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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dynamic_attributes: %w[end_date last_updat start_date]
dynamic_attributes: %w[end_date last_update start_date]

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot a comment line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, thanks

@tsubik tsubik force-pushed the chore/more-cleanup branch 2 times, most recently from 1862faf to 7fdfd48 Compare August 1, 2022 11:19
@tsubik tsubik merged commit 2433203 into develop Aug 1, 2022
@tsubik tsubik deleted the chore/more-cleanup branch August 1, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants