Skip to content
This repository has been archived by the owner on Mar 1, 2022. It is now read-only.

Support for negation and conjunctions on join table in M:M relation #18

Merged
merged 8 commits into from
Dec 4, 2018

Conversation

naz
Copy link
Contributor

@naz naz commented Nov 21, 2018

refs #5
refs #7

This was an attempt to expand existing conjunctions but ended up as a cleanup and extension of existing test cases. The main operation that was missing and has been added is negation and conjunctions support that used joining table in it's filter, e.g:

{
        {
            $and: [
                {
                    'tags.slug': {
                        $ne: 'classic'
                    }
                },
                {
                    'posts_tags.sort_order': 0
                }
            ]
        },
        {
            featured: true
        }
    ]
}

The rest of M:M cases now seems to be covered.

@naz naz changed the title [WIP] Test suite for many-to-many relations cleanup and expansion [WIP] Support for negation and conjunctions on join table in M:M relation Nov 27, 2018
lib/convertor.js Outdated
@@ -47,10 +48,9 @@ class MongoToKnex {
* Determine if statement lives on parent table or if statement refers to a relation.
*/
processStatement(column, op, value) {
const columnParts = column.split('.');

const [tableName, columnName] = column.split('.');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved the naming a bit as it was hard to see right away what columnParts[0] and columnParts[1] actually mean

Copy link
Member

Choose a reason for hiding this comment

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

This is an obvious improvement to me - I would just push this commit (and any like it) right onto master.

Copy link
Member

Choose a reason for hiding this comment

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

Same goes for all the stuff that's just rejigging tests on its own without any related code changes.

return query
.select()
.then((result) => {
// TODO: should this include tags with no tags? the filter is about primary tag != 'classic' so they should count?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs clarification 🤷‍♂️ but seems correct

Copy link
Member

Choose a reason for hiding this comment

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

Tag with no tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

tags with no tags!!! and posts without posts!!!

@naz naz changed the title [WIP] Support for negation and conjunctions on join table in M:M relation Support for negation and conjunctions on join table in M:M relation Nov 27, 2018
@naz
Copy link
Contributor Author

naz commented Nov 28, 2018

@ErisDS if you have time today would love to receive some feedback on this PR. Didn't reorganise the commits, left comments instead :)

@naz naz force-pushed the expand-relations-test-suite branch 5 times, most recently from 3350126 to 1cf9b6b Compare November 28, 2018 22:22
@naz naz force-pushed the expand-relations-test-suite branch 2 times, most recently from b117031 to 66c4d41 Compare November 28, 2018 22:56
@naz naz force-pushed the expand-relations-test-suite branch 2 times, most recently from fe4558b to b6ee673 Compare November 28, 2018 23:10
@naz naz force-pushed the expand-relations-test-suite branch from b6ee673 to 4497763 Compare November 28, 2018 23:15
lib/convertor.js Outdated
@@ -96,7 +96,7 @@ class MongoToKnex {
};
}

// CASE: fallback, `status=draft` -> `posts.status`=draft
// CASE: fallback, `status=drat` -> `posts.status`=draft
Copy link
Member

Choose a reason for hiding this comment

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

typo? :trollface:

@ErisDS
Copy link
Member

ErisDS commented Nov 29, 2018

This all looks good to me - I can see what the changes are intended to accomplish and it all makes sense.

🚢 and continue!

@naz naz merged commit 677b0a3 into TryGhost:master Dec 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants