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

refactor: create mixin to share helper functions across repos #597

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gwynndp
Copy link
Collaborator

@gwynndp gwynndp commented Oct 27, 2021

Description

Organization of helper functions to reduce duplication across repositories

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines

What kind of change does this PR introduce?

  • Enhancement

Issue

What is the current behavior?

There are duplicate functions to help build queries for filtering by organization information in the Trees, Planter, and Organization repositories

What is the new behavior?

Can now use a mixin with a repository to import the helper functions from utils.repository-mixin.ts

Steps taken:

  • Created utils.repository-mixin.ts to house helper functions to integrate organization info into filter queries for use by repositories
  • Moved helper functions from Trees, Planter, & Organization repositories to utils.repository-mixin.ts to reduce duplication
  • added parameters to handle the Trees and Planters query builds differently based on different table names for the organization and id fields
  • update the controllers to pass the necessary information for building the queries
  • fix the query for when the organization has a value of null, represented as "Not Set" in the UI dropdown

Breaking change

Does this PR introduce a breaking change?

  • No

@gwynndp
Copy link
Collaborator Author

gwynndp commented Oct 27, 2021

@nmcharlton Do you want me to write tests for the mixin or not to bother because we're moving to microservices?

I'm also not sure about the queries for "Not set" organizations. The queries filtered for null orgs and for any planters with null orgs. I tried working on this because the captures page was always getting the same result as "All" for "Not set", but the numbers aren't adding up.

@gwynndp gwynndp marked this pull request as ready for review October 30, 2021 17:00
Copy link
Collaborator

@nmcharlton nmcharlton left a comment

Choose a reason for hiding this comment

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

I'm inclined to say don't worry about tests, but this logic has caused us a few problems.
If basic tests can be added reasonably easily, it might prevent some headaches should we need to extend the functionality.

src/mixins/utils.repository-mixin.ts Outdated Show resolved Hide resolved
src/mixins/utils.repository-mixin.ts Outdated Show resolved Hide resolved
src/mixins/utils.repository-mixin.ts Outdated Show resolved Hide resolved
.replace(/"/g, '');

whereObjClause.sql = newQueryFields;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nmcharlton
I added this regex to add the modelName back onto each field because our more complex cross-table queries were resulting in "ambiguous" field errors that I couldn't find any other way of resolving. Is there a better way to resolve those errors?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder whether the LoopBack query builders have a way of including the table name in the query.
This looks ok, but won't catch every WHERE clause construction.

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