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

21429 - staff account search for members #3169

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

ochiu
Copy link
Collaborator

@ochiu ochiu commented Dec 5, 2024

Issue #:
bcgov/entity#21429

Description of changes:

  • search_orgs by member name or username support and members payload

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the sbc-auth license (Apache 2.0).

AND memberships.status = 1
AND users.status = 1
AND CONCAT(users.last_name, ' ', users.first_name, ' ', users.username) ILIKE :member_search_text
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Needed to use string subquery due to model cyclic dependencies.
Membership model has an org import.

Copy link
Collaborator

@seeker25 seeker25 Dec 5, 2024

Choose a reason for hiding this comment

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

Haha, this should be refactored to a service preferably and done in ORM.
I see it uses contacts and contact links which have nothing to do with orgs (in the model :( , not your changes .. )

The only spot it's called is from the service too. Service already is 1000 lines, looks crowded too.

image

If this was pay, would definitely have to refactor.

Copy link
Collaborator

@seeker25 seeker25 Dec 5, 2024

Choose a reason for hiding this comment

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

As long as there is a unit test should be ok (if the datamodel changes etc), but I'll leave it up to you to decide

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should make a ticket for the sbc-connect team to break alot of these dependencies as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I started looking at doing a refactor, and it became a larger undertaking then I wanted for the scope of this ticket. Since it was just a subquery I kept it simple.

I agree we should probably have some kind of tech debt ticket to clean this up.

Copy link

sonarqubecloud bot commented Dec 5, 2024

@ochiu ochiu marked this pull request as ready for review December 5, 2024 07:14
@ochiu ochiu merged commit 8111ea2 into bcgov:main Dec 5, 2024
13 of 15 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.

2 participants