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

[SHARED DEV] [#162198] Add filters to Projects tab #4368

Merged
merged 11 commits into from
Jun 24, 2024

Conversation

LeticiaErrandonea
Copy link
Contributor

@LeticiaErrandonea LeticiaErrandonea commented Jun 18, 2024

Release Notes

  • Add filters to Projects tab

Screenshot

When there are results

image

When there are no results

image

Additional Context

  • In the next PR I'll add Project show with order details

Accessibility

  • Did you scan for accessibility issues?
  • Did you check our accessibility goal checklist?
  • Did you add accessibility specs?

@LeticiaErrandonea LeticiaErrandonea marked this pull request as ready for review June 18, 2024 19:39
@LeticiaErrandonea LeticiaErrandonea requested a review from a team as a code owner June 18, 2024 19:39
Copy link
Contributor

@giladshanan giladshanan left a comment

Choose a reason for hiding this comment

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

Looks good, left a few comments/requests

Comment on lines 33 to 35
Projects::Project
.joins(:orders)
.where(orders: { facility_id: @current_facility_id })
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have a scope that does this same thing:

Suggested change
Projects::Project
.joins(:orders)
.where(orders: { facility_id: @current_facility_id })
Projects::Project.for_facility(@current_facility_id )

... but isn't this logic going to include single facility projects as well?
for OrderDetail we have this scope:

  scope :cross_core, lambda {
    joins(order: [:facility, :cross_core_project])
    .where.not(orders: { cross_core_project_id: nil })
  }

... can you add something like that to the Project model?
then we can do:
Projects::Project.cross_core.for_facility(@current_facility_id )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're right! I added a case for specs but those single facility projects don't have any orders attached.

end

def single_facility_projects
Projects::Project
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
Projects::Project
Projects::Project.single_facility.for_facility(@current_facility_id )

...

  scope :single_facility, lambda {
    joins(order: [:facility, :cross_core_project])
    .where(orders: { cross_core_project_id: nil })
  }


search_params = params[searcher_class.key.to_sym]

# Options should not be restricted, they should search over the full order details
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
# Options should not be restricted, they should search over the full order details
# Options should not be restricted, they should search over the full list of projects

@@ -83,6 +88,10 @@ def showing_inactive?

private

def initial_projects
nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to use nil here instead of an empty array? curious why this needs a method of its own? maybe the searcher should have this as an optional input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The searcher uses recursion to filter the previous list of projects, making the input optional would also mean switching the params. I can do that but I wonder if it's the best as it'd make this searcher different from the rest and could be a bit confusing.

It definitely doesn't need to be its own method, I'll fix that.

Copy link
Contributor

@giladshanan giladshanan left a comment

Choose a reason for hiding this comment

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

🎉

@LeticiaErrandonea LeticiaErrandonea merged commit 6ef9a16 into master Jun 24, 2024
3 checks passed
@LeticiaErrandonea LeticiaErrandonea deleted the 162198-projects_tab_cross_core branch June 24, 2024 13:11
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

2 participants