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

feat(app): implement insensitive operators in app #13431

Conversation

bernatvadell
Copy link
Contributor

In relation to PR #11737 , I've implemented the rest of the operators in the application.

CC. @nordcart

@netlify
Copy link

netlify bot commented May 20, 2022

👷 Deploy request for directus-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit fd840f5

@rijkvanzanten
Copy link
Member

@jaycammarano Can you look into spinning up some tests for the generate-joi util that is touched in this PR? I'm not quite sure if we can easily test the generated Joi schema, so we might want to treat it as an integration test by providing known valid/invalid payloads against the generated schema in order to test the generation of the schema.

@jamescammarano
Copy link
Contributor

jamescammarano commented May 27, 2022

@jaycammarano Can you look into spinning up some tests for the generate-joi util that is touched in this PR? I'm not quite sure if we can easily test the generated Joi schema, so we might want to treat it as an integration test by providing known valid/invalid payloads against the generated schema in order to test the generation of the schema.

Do we want to reorganize the shared package to have a tests folder the way we are doing with the other packages?

@rijkvanzanten
Copy link
Member

@jaycammarano Yeah lets make that consistent 👍🏻

Comment on lines +86 to +88
4. Leave the other options at default. Click <span mi btn>arrow*forward</span> and the **"Optional System Fields"** menu
will open.\
_Keep the values in this menu at the default, toggled off, for now. You can adjust them later._
\_Keep the values in this menu at the default, toggled off, for now. You can adjust them later.*
Copy link
Contributor

Choose a reason for hiding this comment

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

@rijkvanzanten @jaycammarano

Something weird is going on here, and i can't figure it out. Looks like every time we make a commit, arrow_forward compiles to arrow*forward.... In the PR when I wrote this doc, I used arrow\_forward and that compiled into arrow_forward after the commit... But from this, it appears all commits appear to impact it.

This is what causes that weirdness on line 88. As you can see, it was written _abc_ and then gets compiled to _abc* when 86 goes haywire.

I spent at least 30 min trying to see if there was a * or _ mismatch somewhere before and couldn't spot one.

Any fast ideas on what's up here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's due to every time we are committing, this hook runs lint-staged:

directus/package.json

Lines 63 to 65 in 4bab194

"simple-git-hooks": {
"pre-commit": "npx lint-staged"
},

which in turns runs:

directus/package.json

Lines 66 to 69 in 4bab194

"lint-staged": {
"*.{js,ts,vue}": "eslint --fix",
"*.{md,yaml}": "prettier --write"
},

with line 68 being the relevant cause of "weirdness" here since it's related to .md files. Did a brief search and it seems prettier/prettier#3837 is similar to what you are experiencing. Were you using a similar syntax in the past?

Not sure what is the appropriate solution (besides the prettier ignore suggested in the thread at the moment), but hope this at least helps to pinpoint it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @azrikahar !
Based on that bug you sent, it looks like I probably have an extra * somewhere....
I will track it down.

@erondpowell
Copy link
Contributor

@bernatvadell @jaycammarano
I smell a minor docs update coming at the end of all this :octocat:
Please tag me when this is ready so I can make that play!
(I will try to keep up as well, but a tipoff from the main geezers is always helpful 😄 )

@jamescammarano
Copy link
Contributor

There is a push to use @directus/shared/utils for generate-joi instead of the one in app/utils. Would you be willing to make this shift?

@rijkvanzanten
Copy link
Member

There is a push to use @directus/shared/utils for generate-joi instead of the one in app/utils. Would you be willing to make this shift?

This is in-works at #13596. Once that's cleared up, we can merge this PR 👍🏻

@nordcart
Copy link

There is a push to use @directus/shared/utils for generate-joi instead of the one in app/utils. Would you be willing to make this shift?

This is in-works at #13596. Once that's cleared up, we can merge this PR 👍🏻

I know that you guys have your hands tied up but #13596 will be in the next release, is it somehow possible this PR could also be in the next release?

@rijkvanzanten
Copy link
Member

@nordcart This PR needs some tweaking to match the cleaned up structure of the generate-joi and seeing the next release is in two days, and we're a maintainer down do to covid (hope you'll feel better soon @br41nslug! 🦠), I can't promise that this'll make it in by then.

@nordcart
Copy link

nordcart commented Sep 6, 2022

@nordcart This PR needs some tweaking to match the cleaned up structure of the generate-joi and seeing the next release is in two days, and we're a maintainer down do to covid (hope you'll feel better soon @br41nslug! 🦠), I can't promise that this'll make it in by then.

Any news on this?

@nordcart
Copy link

Maybe this could make it to the next release? 😶‍🌫️

@nordcart
Copy link

🤔

@rijkvanzanten
Copy link
Member

Heya, thanks for your PR! Our small team has been attempting to work through a backlog of external contributions for the better part of a year, but in that time many have become too stale to merge or the change is no longer desired/relevant..
We've updated the contributing guidelines and are working on making more resources available to make sure that doesn’t happen again 😬 In the meantime, we'll have to close this PR for now..

@nordcart
Copy link

Heya, thanks for your PR! Our small team has been attempting to work through a backlog of external contributions for the better part of a year, but in that time many have become too stale to merge or the change is no longer desired/relevant.. We've updated the contributing guidelines and are working on making more resources available to make sure that doesn’t happen again 😬 In the meantime, we'll have to close this PR for now..

Thank you for the info, and for the great work you do at the open source world!
I´ve been following this for 2 years, starting with issue #5996 .

Is there however some other way to filter o2m where only for example authors with an article "test" is shown?

Take the following minimal example:

authors & articles
articles.author -> authors.id

Fetch all authors that have an article where title IS NOT "Test"

The following SQL is used

SELECT
	"authors"."id",
	"authors"."name"
FROM
	"authors"
WHERE
	"authors"."id" in(
		SELECT
			"articles"."author" AS "author"
		FROM
			"articles"
		WHERE
			NOT "articles"."title" = 'Test'
	)
ORDER BY
	"authors"."id" ASC

However, this still fetches the top level author as soon as they have any other article that matches the search query 🤔

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants