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

Remove OpenStructs from code base #4832

Closed
wants to merge 5,905 commits into from

Conversation

coalest
Copy link
Collaborator

@coalest coalest commented Dec 7, 2024

Doesn't resolve any issue.

Description

While thinking about performance, I noticed some usage of OpenStructs in the app. While they are only used in a few places, let's get rid of them.

From the official documentation:

An OpenStruct utilizes Ruby's method lookup structure to find and define the
necessary methods for properties. This is accomplished through the methods
method_missing and define_singleton_method.

This should be a consideration if there is a concern about the performance of
the objects that are created, as there is much more overhead in the setting
of these properties compared to using a Hash or a Struct.
Creating an open struct from a small Hash and accessing a few of the
entries can be 200 times slower than accessing the hash directly.

...

For all these reasons, consider not using OpenStruct at all.

The performance impact in our application will be small, but I don't see a strong reason to keep them around.

Type of change

  • Refactor

How Has This Been Tested?

Strictly internal refactor. No additional tests added.

lit-poks and others added 30 commits October 4, 2024 15:35
Change State too Status in view

status in index
Bumps [redis](https://github.com/redis/redis-rb) from 5.2.0 to 5.3.0.
- [Changelog](https://github.com/redis/redis-rb/blob/master/CHANGELOG.md)
- [Commits](redis/redis-rb@v5.2.0...v5.3.0)

---
updated-dependencies:
- dependency-name: redis
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Brock Wilcox <[email protected]>
…m Categories changes to a yes/no! Obviously wrong! (#4668)

* fix(partner_groups_controller): assign item_categories when create or edit params are invalid

* fix: typo
Bumps [puma](https://github.com/puma/puma) from 6.4.2 to 6.4.3.
- [Release notes](https://github.com/puma/puma/releases)
- [Changelog](https://github.com/puma/puma/blob/master/History.md)
- [Commits](puma/puma@v6.4.2...v6.4.3)

---
updated-dependencies:
- dependency-name: puma
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Remove filtering for active attribute on children query

* Add spec that tests for child item requests
…-status

Change State to Status on Distribution Table
Add partner group to organization. Add new partner for seed
* validate the presence of business name for a vendor

add validation for business name on vendor model, skip validation in provideable when the model is a vendor to prevent duplicate errors on invalid form submission, specs for both changes

* create migration to update a vendor's business name to its contact name if business name is blank

* move product drive participant validations out of provideable and into model instead of skipping when the provideable model is vendor

* fix arg name
* Change button from 'Promote to Admin' to 'Edit User'

* Replace system spec with request spec for link for editing user

* Replace system spec with request spec for link for editing user
…n beside Add a Partner link. Add system tests to ensure the Partner cta section has 2 links. (#4710)

Co-authored-by: Athira Kadampatta <[email protected]>
# Conflicts:
#	app/controllers/distributions_controller.rb
#	spec/rails_helper.rb
* Add view password functionality to login form

* refactored the password visiblity toggle into a Stimulus controller
user docs cont v:  draft inventory section of user guide
User docs cont vi: draft exports section of user guide
Fix inventory seeding for storage location from donations
awwaiid and others added 9 commits December 15, 2024 11:14
build(deps): bump rails from 7.1.3.4 to 7.2.2
* Fix unexpected issued_at behavior when date field empty.

Previously before_save and before_create callbacks set the value for issued_at
created_at end of day if no datetime was provided. This prevented records being
saved with no issue date. But it caused unexpected behavior when creating or
updating a donation/purchase/distribution with an empty issued_at field,
silently changing the issued_at date to something potentially incorrect.

* Fix missing partner dropdown options of distribution update fail

* Revert partner list fix
* Fix unitialized money_rails constant error

* Fix require statement file_path and edit comment
This test expects the requests in the export to be in a particular order,
but no order is applied to the requests passed in and no order is defined
in the ExportCreateService itself, hence the flakiness.

Example failure: https://github.com/rubyforgood/human-essentials/actions/runs/12353561322/job/34472934783#step:7:1433
…#4801)

* Migration for partner agency cleanup re issue 4241.  Necessary step, but does not complete that issue.

* save! instead of save

* save! instead of save
@coalest coalest force-pushed the replace-open-structs branch from 107ec8b to eb6007c Compare December 18, 2024 11:50
@coalest coalest changed the title Remove OpenStructs except in test suite Remove OpenStructs from code base Dec 18, 2024
@coalest
Copy link
Collaborator Author

coalest commented Dec 18, 2024

Made the requested changes, so this is ready for another round of review.

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

One suggestion.

super
end

alias_method :success?, :success
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a success attribute? Why wouldn't we just define a method?

def success? = error.nil?

Copy link
Collaborator Author

@coalest coalest Dec 20, 2024

Choose a reason for hiding this comment

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

Yea I was debating this internally and wasn't sure which was better. Will fix.

coalest and others added 10 commits December 19, 2024 18:52
* Fix flaky family requests system spec

* Fix flaky system partners children spec
…to 6.5.0 (#4813)

* build(deps-dev): bump factory_bot_rails from 6.4.3 to 6.4.4

Bumps [factory_bot_rails](https://github.com/thoughtbot/factory_bot_rails) from 6.4.3 to 6.4.4.
- [Release notes](https://github.com/thoughtbot/factory_bot_rails/releases)
- [Changelog](https://github.com/thoughtbot/factory_bot_rails/blob/main/NEWS.md)
- [Commits](thoughtbot/factory_bot_rails@v6.4.3...v6.4.4)

---
updated-dependencies:
- dependency-name: factory_bot_rails
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* Fix merge conflicts with main

* Update puma to 6.5.0

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Daniel Orner <[email protected]>
Co-authored-by: Brock Wilcox <[email protected]>
Co-authored-by: Cory Streiff <[email protected]>
We were saving a new distribution (which causes a redirect to that
distribution's show page) and then immediately clicking on the distributions
index page (which redirects as well). This introduced a race condition that
was flaky when we tried to click a link that didn't exist on the page.
We were expecting the `family_zipcodes_list` to have a certain order
but no order is guaranteed in the request.
* lowered pagination threshold in dev and staging using kaminari

* Trigger Build
@coalest coalest force-pushed the replace-open-structs branch from e032fda to ef0878a Compare December 21, 2024 15:44
@cielf
Copy link
Collaborator

cielf commented Dec 21, 2024

I'm going to have trouble figuring out what to test on this -- is there a way I can get just the relevant commits?

@coalest coalest closed this by deleting the head repository Dec 21, 2024
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.