3351 add family archive #3623

grantmca · 2023-05-26T23:59:48Z

Checklist:

I have performed a self-review of my own code,
I have added tests that prove my fix is effective or that my feature works,
New and existing unit tests pass locally with my changes ("bundle exec rake"),
Title include "WIP" if work is in progress.

Resolves #3351

Description

Guide questions:

What motivated this change (if not already described in an issue)?
--The ability to archive families no longer use to help users not have to scroll through large list
--Increases usability for the user, provides additional info on reports

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

You can validate the change by switching some families to archived in the dev env and ensuring they do not show up by default, additionally check that any children associated with the families are also archived. Also validate that when "Include Archived" is checked that all families are shown on the index screen

Screenshots

To make things look good I centered both the checkbox on families and on children filters so they match

* Updated the view, controller and test to use the new column * Added auto archive of children on families archive

* Added scope to Families with unit tests * Added default filter to families controller * Updated the CSV output on the families controller * Added `Include Archived?` option on the families view * Added request test to ensure default only returns the non-archived * Added test for `Include Archived?` check on index

dorner

Thanks for the PR! Added some comments and suggestions.

app/models/partners/family.rb

app/controllers/partners/families_controller.rb

db/migrate/20230430025441_add_archive_to_families.rb

spec/models/partners/family_spec.rb

* Extracted archiving the children of a family to a service object that is called in the family controller and took the funciton and callback off of the family model * Updated and added unit test * Updated the migration to remove backfill migration

…validations

grantmca · 2023-06-14T12:42:32Z

I think I have addressed all of the changes, I might need to rebase but should be set

dorner

Getting closer! Had some more comments.

app/controllers/partners/families_controller.rb

app/models/partners/family.rb

app/controllers/partners/families_controller.rb

app/services/partners/archive_family_children.rb

db/migrate/20230430025441_add_archive_to_families.rb

grantmca · 2023-06-19T01:51:06Z

Getting closer! Had some more comments.

Thank you! I'll get on it

updated it to module level function. I also updated the migration table to creat the column and set nill and default all on one line

grantmca · 2023-07-15T20:29:37Z

I moved forward with all of the changes to the service class and the migration. I played around with updating the family controller for a while to make it work with bools but I did some investigation into filterrific and it when you pass in 'false' as a query parameter to filteriffic it translates that false to '0' and when you pass in true it is passed through as true. 0 is truthy in ruby and the True object does not have the method zero? on it so there is not good concise way to write the scope. I think the way it is now it the best option and the it follows the pattern of the children model as well.

dorner · 2023-07-16T13:57:52Z

@grantmca I'm not sure I understand this. Filterrific parameters get sent to the controller. The controller can make whatever changes it has to do to those parameters rather than passing them blindly to the service object or the model. I'd rather see those changes happen in the controller than downstream.

grantmca · 2023-07-18T03:15:45Z

@grantmca I'm not sure I understand this. Filterrific parameters get sent to the controller. The controller can make whatever changes it has to do to those parameters rather than passing them blindly to the service object or the model. I'd rather see those changes happen in the controller than downstream.

I apologize for any confusion caused by my previous explanation. Here's a more detailed breakdown: the view sends either '0' or '1' as the params for the checkbox to the controller. Within the controller, I implemented and tested logic that cast this '0' or '1' to a boolean value and then pass this boolean to the 'filterrific' initializer. The gem 'filterrific' provides us with the service object @filterrific, and its method @filterrific.find receives the filterrific parameters from the controller. Using these parameters and a scope defined on the model, the method returns the desired results. However, there's is a catch: the "filterrific" service object performs some parameter data cleanup before passing the parameters to the model's scope. One such cleanup action is converting 'false' to 0 before passing it to the model's scope. Since the model's scope might expect a boolean value, this can lead to converting the scope just for it to be converted back to 0. I am open to any suggestions on how to best handle this case! Let me know if you need any further adjustments!

dorner · 2023-07-18T20:57:24Z

I think the issue might be because it's using the string "false" instead of a Boolean false. Can you try casting the string to a boolean and see what happens?

grantmca · 2023-07-20T02:47:45Z

I think the issue might be because it's using the string "false" instead of a Boolean false. Can you try casting the string to a boolean and see what happens?

I am sorry I should of used backticks instead of single quotes, I am passing the bool false into the params when I was getting that response. I am struggling to figure out why this is happening. I have unpacked the gem and I am digging into it to try and see if I can come up with any reason, I will keep you updated with what I find

dorner · 2023-07-21T19:33:08Z

Please do, thanks. This behavior really doesn't make any sense.

grantmca · 2023-08-06T19:24:18Z

Please do, thanks. This behavior really doesn't make any sense.

This seems to maybe be a bug on Filterfific's end. On this line you can see that in the define_and_assign_attr_accessors_for_each_filter goes through each of the params for each filter and before assigning them as attr_accessor it checks each of them to see if they are present? and if not it will not assign them. In our case false.present? is false because false is treated as a non-present value so the parameter is not being passed into the scope. This does not seem like the intended behavior for the service obj so I am going to reach out on the filterrific gem and maybe create a PR to fix this issue. Given this info what would you recommend on this PR? @dorner

dorner · 2023-08-10T00:18:52Z

So it looks like this is the suggested approach: jhund/filterrific#9

In general I'd try to keep the 0/1 check to a single place. If you have to massage the parameters to turn the 0/1 into true/false, I'd do it in a method or callback before any other methods are called.

grantmca · 2023-08-26T20:12:59Z

So it looks like this is the suggested approach: jhund/filterrific#9

In general I'd try to keep the 0/1 check to a single place. If you have to massage the parameters to turn the 0/1 into true/false, I'd do it in a method or callback before any other methods are called.

I went ahead and opened a PR for this on filterrific jhund/filterrific#224 @dorner let me know how we should proceed form here.

dorner · 2023-08-27T18:37:58Z

@grantmca did you try this approach? jhund/filterrific#9 (comment) We could use that while we wait :)

* Updated the view, controller and test to use the new column * Added auto archive of children on families archive

* Added scope to Families with unit tests * Added default filter to families controller * Updated the CSV output on the families controller * Added `Include Archived?` option on the families view * Added request test to ensure default only returns the non-archived * Added test for `Include Archived?` check on index

* Extracted archiving the children of a family to a service object that is called in the family controller and took the funciton and callback off of the family model * Updated and added unit test * Updated the migration to remove backfill migration

…validations

updated it to module level function. I also updated the migration table to creat the column and set nill and default all on one line

… filterrific

…51-add-family-archive

grantmca · 2023-09-06T03:44:06Z

I think that we are all set now! @dorner, I updated the scope to follow the example on the issue you linked. Let me know what you think!

dorner

Probably as good as it's going to get - but I think there's a typo in the migration file?

db/migrate/20230430025441_add_archive_to_families.rb

dorner · 2023-09-14T13:17:10Z

Thanks for all your patience with this! In it goes!

grantmca added 3 commits May 26, 2023 13:40

Added archive column to the Families column

cce2de1

* Updated the view, controller and test to use the new column * Added auto archive of children on families archive

Fixed some formatting with for respec

1aa1f0b

grantmca force-pushed the 3351-add-family-archive branch from b3155d1 to 1aa1f0b Compare May 27, 2023 00:29

grantmca changed the title ~~WIP: 3351 add family archive~~ 3351 add family archive May 27, 2023

Merge branch 'main' into 3351-add-family-archive

13a2aa4

dorner requested changes May 28, 2023

View reviewed changes

grantmca added 6 commits May 29, 2023 17:29

Addressed rubocop formatting issues

ffeb73c

Merge branch 'main' into 3351-add-family-archive

f784e30

Added ignore for update_all for rubocop since the Child class has no …

4a7315c

…validations

Merge branch 'main' into 3351-add-family-archive

def4be2

Quick Fix to update the rubocop issue with update_all

c07a385

dorner requested changes Jun 16, 2023

View reviewed changes

grantmca added 2 commits July 15, 2023 15:03

Renamed the archive_family_children service object to update_family and

f278910

updated it to module level function. I also updated the migration table to creat the column and set nill and default all on one line

Missed a whitespace after the UpdateFamily service module

676015b

grantmca mentioned this pull request Aug 26, 2023

Update the params_set to work with bool values jhund/filterrific#224

Open

Added archive column to the Families column

3c64d8b

* Updated the view, controller and test to use the new column * Added auto archive of children on families archive

grantmca added 12 commits September 3, 2023 22:39

Fixed some formatting with for respec

ea8dddd

Addressed rubocop formatting issues

73f6c20

Added ignore for update_all for rubocop since the Child class has no …

b5ed4e5

…validations

Quick Fix to update the rubocop issue with update_all

4d99498

Renamed the archive_family_children service object to update_family and

2f1e7ae

updated it to module level function. I also updated the migration table to creat the column and set nill and default all on one line

Missed a whitespace after the UpdateFamily service module

039084b

Updated the scope to follow the pattern recommended for checkboxes on…

045e21d

… filterrific

Merge remote-tracking branch 'origin/3351-add-family-archive' into 33…

fbf0310

…51-add-family-archive

Merge branch 'main' into 3351-add-family-archive

05b3bf7

Fixed some whitespace for rubocop

14f248e

grantmca requested a review from dorner September 8, 2023 03:16

dorner requested changes Sep 8, 2023

View reviewed changes

db/migrate/20230430025441_add_archive_to_families.rb Outdated Show resolved Hide resolved

grantmca added 2 commits September 8, 2023 22:11

Merge branch 'main' into 3351-add-family-archive

816a514

Fixed a small typo on the migration

73b5110

grantmca requested a review from dorner September 9, 2023 03:18

Merge branch 'main' into 3351-add-family-archive

ef2d162

dorner approved these changes Sep 14, 2023

View reviewed changes

dorner merged commit 4ff7b49 into rubyforgood:main Sep 14, 2023

scooter-dangle added a commit that referenced this pull request Sep 17, 2023

fix: bad schema version introduced in #3623

e0bd0b2

scooter-dangle mentioned this pull request Sep 17, 2023

fix: bad schema version introduced in #3623 #3887

Merged

scooter-dangle added a commit that referenced this pull request Sep 17, 2023

fix: bad schema version introduced in #3623 (#3887)

339436f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3351 add family archive #3623

3351 add family archive #3623

grantmca commented May 26, 2023

dorner left a comment

grantmca commented Jun 14, 2023

dorner left a comment

grantmca commented Jun 19, 2023

grantmca commented Jul 15, 2023

dorner commented Jul 16, 2023

grantmca commented Jul 18, 2023

dorner commented Jul 18, 2023

grantmca commented Jul 20, 2023

dorner commented Jul 21, 2023

grantmca commented Aug 6, 2023

dorner commented Aug 10, 2023

grantmca commented Aug 26, 2023

dorner commented Aug 27, 2023

grantmca commented Sep 6, 2023

dorner left a comment

dorner commented Sep 14, 2023

3351 add family archive #3623

3351 add family archive #3623

Conversation

grantmca commented May 26, 2023

Checklist:

Description

Type of change

How Has This Been Tested?

Screenshots

dorner left a comment

Choose a reason for hiding this comment

grantmca commented Jun 14, 2023

dorner left a comment

Choose a reason for hiding this comment

grantmca commented Jun 19, 2023

grantmca commented Jul 15, 2023

dorner commented Jul 16, 2023

grantmca commented Jul 18, 2023

dorner commented Jul 18, 2023

grantmca commented Jul 20, 2023

dorner commented Jul 21, 2023

grantmca commented Aug 6, 2023

dorner commented Aug 10, 2023

grantmca commented Aug 26, 2023

dorner commented Aug 27, 2023

grantmca commented Sep 6, 2023

dorner left a comment

Choose a reason for hiding this comment

dorner commented Sep 14, 2023