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

Simplify and refactor SiteData, removing unused code #1989

Merged
merged 9 commits into from
Mar 4, 2024

Conversation

nimmolo
Copy link
Contributor

@nimmolo nimmolo commented Mar 2, 2024

Trying to get a handle on the confusing logic of this class, prior to writing a table to store the data.

  1. There's a bunch of unused code for two planned but never implemented contribution metrics: observations_with_voucher and observations_without_voucher. These cases required SiteData to account for much more complex logic. The logic somehow was implemented, but is always bypassed in code execution. I'm getting rid of it; it makes the code harder to read and refactor. Those fields can be rebuilt when the code here is simpler, if desired.
  2. There is code for getting all data for all users, and for the whole site, that is never implemented, thankfully, because it crashes if run locally.
  3. The main queries are composed in strings of raw SQL. This works, but I'm going to try to rewrite the whole thing in AR.

Copy link
Member

@pellaea pellaea left a comment

Choose a reason for hiding this comment

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

Looking so much better!

# !(obs.specimen && obs.notes.to_s.length >= 10 &&
# obs.thumb_image_id.to_i.positive?)
# end
# }.freeze

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to erase these completely, they'd be easy to reconstruct anyway.

case mode
when :del
-weight
when :chg
get_weight_change(obj, field)
0 # get_weight_change(obj, field)
Copy link
Member

Choose a reason for hiding this comment

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

What is this case for? Do we ever actually use it?

# def self.get_weight_change(obj, new_field)
# old_field = new_field
# FIELD_WEIGHTS[new_field] - FIELD_WEIGHTS[old_field]
# end
Copy link
Member

Choose a reason for hiding this comment

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

Good riddance!!!!

# end

@user_id = id.to_i
user = User.find(id)

# Prime @user_data structure.
@user_data = {}
Copy link
Member

Choose a reason for hiding this comment

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

This is fishy. Either this is never called on multiple users, in which case @user_data doesn't need to be a hash, or it is called on multiple users, in which case shouldn't this is @user_data ||= {}? As it is, it clobbers the hash each time and sticks a single user in it.

@coveralls
Copy link
Collaborator

coveralls commented Mar 2, 2024

Coverage Status

coverage: 94.384% (-0.04%) from 94.421%
when pulling 4f0fce9 on nimmo-simplify-site-data
into 714c8f3 on main.

@nimmolo nimmolo marked this pull request as ready for review March 4, 2024 06:01
# Add some extra stats.
@site_data[:observed_taxa] = Observation.distinct.count(:name_id)
@site_data[:listed_taxa] = Name.count

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into SiteData

@nimmolo nimmolo merged commit c837c7d into main Mar 4, 2024
3 of 5 checks passed
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.

3 participants