feat: Filter pacticipant name on index page #446

bangn · 2021-06-04T15:20:25Z

This is the look and feel

bangn · 2021-06-04T15:23:36Z

public/stylesheets/index.css

+div.top-of-group {
+  margin-top: 20px;
+  margin-bottom: 20px;
+}


When I test it locally, looks like the change in the index.css file does not
get reflected
Copy those changes and put into Firefox css style editor result in the image in
the PR description

Im not sure what I did wrong which made the index.css changes not get copied
to the server asset. Any hint?

Hm, I can't think of anything to be honest.

It is fixed now. Im not sure why it did not work before 🤷

bangn · 2021-06-04T15:25:22Z

spec/lib/pact_broker/ui/controllers/index_spec.rb

+                end
+              end
+
+              context "when it is NOT blank and exist" do


I would prefer not having to many level of indentation, hence the and in the
description. Please let me know if you think otherwise

bangn · 2021-06-07T07:36:42Z

This is to fix #318

bethesque · 2021-06-08T04:16:32Z

Can we re-think the search form? How about a single field where you can type in anything, and it does a partial match with either consumer or provider names? I feel like having to type in the exact consumer/provider name might be a bit fiddley.

eg. Search: foo bar would return the "Foo/Bar" pact as well as the "Fooby/Barrel" pact, as well as the "foo-bar/xyz" pact.

This is how the Pactflow search bar works.

What do you think?

bethesque · 2021-06-08T05:00:44Z

Like this except with form submission instead of dynamic.

bethesque · 2021-06-08T05:01:21Z

I did not expect that gif to be so big 😆

bangn · 2021-06-08T18:02:40Z

Can we re-think the search form? How about a single field where you can type in anything, and it does a partial match with either consumer or provider names? I feel like having to type in the exact consumer/provider name might be a bit fiddley.

Yes agree 👍

Search: foo would return the "Foo/Bar" pact as well as the "Fooby/Barrel" pact, as well as the "foo-bar/xyz" pact.

This can be archieved by having some sort of regex matching. However

eg. Search: foo bar would return the "Foo/Bar" pact as well as the "Fooby/Barrel" pact, as well as the "foo-bar/xyz" pact.

This may need some kind of fuzzy search matching. This gem comes to my mind https://github.com/kiyoka/fuzzy-string-match.
It gives a better user experience with a cost of extra dependency.

Im leaning toward using some kind of regex matching instead of adding extra dependency.

What do you think?

bethesque · 2021-06-09T03:18:06Z

I think let's just start with a substring, ignore case match and get feedback on that.

bethesque · 2021-06-09T03:39:07Z

You can do a substring, case ignore match using the Sequel gem like so:

# I found out the hard way that underscores are wildcard characters in SQL, so need to be escaped
[7] pry(main)> terms = ['foo','bar'].collect{ | term| term.gsub("_", "\\_") } 
[9] pry(main)> PactBroker::Domain::Pacticipant.where( Sequel.|(* terms.collect{ | term | Sequel.ilike(:name, "%#{term}%")} ))
=> #<Sequel::SQLite::Dataset: "SELECT * FROM `pacticipants` WHERE ((UPPER(`name`) LIKE UPPER('%foo%') ESCAPE '\\') OR (UPPER(`name`) LIKE UPPER('%bar%') ESCAPE '\\'))">

Then find any pact that has either the consumer or provider in that list of pacticipants.

bethesque · 2021-06-09T05:31:09Z

lib/pact_broker/index/service.rb

-        error_messages = []
-
-        pacticipant = pacticipant_service.find_pacticipant_by_name(pacticipant_name)
+      def self.filter_item_by_pacticipant_name(index_items, pacticipant_name)


The query to generate the index results is quite intensive, and can be slow. It's better to filter at the SQL level, not do the entire query and then filter in memory.

Find the matching pacticipants as per the example that I posted earlier, get their IDs, then update the base query to do something like:

query = query.where( Sequel.|( { consumer_id: pacticipant_ids}, { provider_id: pacticipant_ids} ) )

bethesque · 2021-06-14T23:29:33Z

lib/pact_broker/index/service.rb

+        query
+      end
+
+      def self.pacticipant_name_query(pacticipant_name)


This is getting closer! But it's still filtering a really big slow dataset (the entire index query) on a string match, rather than finding the pacticipants (a really small query) on a string match, and then being able to set the exact IDs of the consumer and provider in the big query (which will make it super quick). What's your thinking behind that?

bethesque · 2021-06-14T23:29:51Z

lib/pact_broker/index/service.rb

+        query
+      end
+
+      def self.pacticipant_name_query(pacticipant_name)


This is getting closer! But it's still filtering a really big slow dataset (the entire index query) on a string match, rather than finding the pacticipants (a really small query) on a string match, and then being able to set the exact IDs of the consumer and provider in the big query (which will make it super quick). What's your thinking behind that?

What's your thinking behind that?

I was thinking doing 2 round trips to the database is a bit inefficient.
However, I think I'm wrong as you said filtering string match with ilike is also an expensive query

I'll do it the way you suggested and probably find the way to optimise the current index query on a different issue/PR :)

Thanks for being patient with me :)

Attemp in 8fd82a5

The new SQL query looks like below

SELECT pact_publications.* FROM pact_publications INNER JOIN pacticipants AS consumers ON (pact_publications.consumer_id = consumers.id) INNER JOIN pacticipants AS providers ON (pact_publications.provider_id = providers.id) INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id) WHERE ((pact_publications.id IN (SELECT p.* FROM (SELECT pact_publications.id FROM pact_publications INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id) LEFT JOIN (SELECT consumer_id, provider_id, "order" FROM pact_publications INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id)) AS pp2 ON ((pact_publications.consumer_id = pp2.consumer_id) AND (pact_publications.provider_id = pp2.provider_id) AND (pp2.order > cv.order)) WHERE (pp2.order IS NULL)) AS p INNER JOIN latest_pact_publication_ids_for_consumer_versions AS lp ON (lp.pact_publication_id = p.id))) AND (pact_publications.id IN (SELECT pact_publications.id FROM pact_publications INNER JOIN pacticipants AS consumers ON (pact_publications.consumer_id = consumers.id) INNER JOIN pacticipants AS providers ON (pact_publications.provider_id = providers.id) WHERE ((providers.name ILIKE '%test%' ESCAPE '\') OR (providers.name ILIKE '%ap\_tesp%' ESCAPE '\') OR (consumers.name ILIKE '%test\_d%' ESCAPE '\') OR (consumers.name ILIKE '%appdf%' ESCAPE '\')))))

Please let me know how you think.
There is a failed test in app_spec.rb which Im a bit baffled to understand. Will look at it later after you are happy with the filtering :)

This is the part that gets added to a big index query

AND (pact_publications.id IN (SELECT pact_publications.id FROM pact_publications INNER JOIN pacticipants AS consumers ON (pact_publications.consumer_id = consumers.id) INNER JOIN pacticipants AS providers ON (pact_publications.provider_id = providers.id) WHERE ((providers.name ILIKE '%test%' ESCAPE '\') OR (providers.name ILIKE '%ap\_tesp%' ESCAPE '\') OR (consumers.name ILIKE '%test\_d%' ESCAPE '\') OR (consumers.name ILIKE '%appdf%' ESCAPE '\')))))

The problem with this approach is that it has to do all the joins, creating a big dataset, then remove lines by filtering on the pacticipant names afterwards.

By doing the query to get the pacticipant ids first, and only joining those consumers, then the dataset never gets big in the first place. Ideally, we want a query that looks like:

SELECT pact_publications.id FROM pact_publications INNER JOIN pacticipants AS consumers ON (pact_publications.consumer_id = consumers.id and pact_publications.consumer_id in [<the ids from the first query>]) INNER JOIN pacticipants AS providers ON (pact_publications.provider_id = providers.id and providers.id in [<the ids from the first query>])

I've learned that making an extra round trip to the database is worth the trade off when you can significantly cut down on the amount of data being processed in a subsequent query.

Sorry, I put those IN statements in the wrong part of the query.

SELECT pact_publications.* FROM pact_publications INNER JOIN pacticipants AS consumers ON (pact_publications.consumer_id = consumers.id) INNER JOIN pacticipants AS providers ON (pact_publications.provider_id = providers.id) INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id) WHERE ((pact_publications.id IN (SELECT p.* FROM (SELECT pact_publications.id FROM pact_publications --- filter here WHERE ( pact_publications.consumer_id IN [...] OR pact_publications.provider_id IN [...]) INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id) LEFT JOIN (SELECT consumer_id, provider_id, "order" FROM pact_publications INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id)) AS pp2 ON ((pact_publications.consumer_id = pp2.consumer_id) AND (pact_publications.provider_id = pp2.provider_id) AND (pp2.order > cv.order)) WHERE (pp2.order IS NULL)) AS p INNER JOIN latest_pact_publication_ids_for_consumer_versions AS lp ON (lp.pact_publication_id = p.id))) AND (pact_publications.id IN (SELECT pact_publications.id FROM pact_publications --- filter here WHERE ( pact_publications.consumer_id IN [...] OR pact_publications.provider_id IN [...]) INNER JOIN pacticipants AS consumers ON (pact_publications.consumer_id = consumers.id) INNER JOIN pacticipants AS providers ON (pact_publications.provider_id = providers.id) )

See how that's filtering the base table before it gets joined, rather than on a joined table after it gets joined?

Fixed in 80388e9

This is the final SQL. As we already got the pacticipant ids, we can use them in a subquery too. I think it will also boost up the index query.

SELECT pact_publications.* FROM pact_publications INNER JOIN pacticipants AS consumers ON (pact_publications.consumer_id = consumers.id) INNER JOIN pacticipants AS providers ON (pact_publications.provider_id = providers.id) INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id) WHERE (pact_publications.id IN (SELECT p.* FROM (SELECT pact_publications.id FROM pact_publications INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id) LEFT JOIN (SELECT consumer_id, provider_id, ORDER FROM pact_publications INNER JOIN versions AS cv ON (pact_publications.consumer_version_id = cv.id) --- use pacticticpant ids in subquery here WHERE (pact_publications.id IN (1, 2, 3, 4))) AS pp2 ON ((pact_publications.consumer_id = pp2.consumer_id) AND (pact_publications.provider_id = pp2.provider_id) AND (pp2.order > cv.order)) --- filter by pacticipant ids here WHERE ((pact_publications.id IN (1, 2, 3, 4)) AND (pp2.order IS NULL))) AS p INNER JOIN latest_pact_publication_ids_for_consumer_versions AS lp ON (lp.pact_publication_id = p.id)));

bangn · 2021-06-17T12:57:53Z

sqlite action failed due to

Im not sure why it is failing

bethesque · 2021-06-20T22:42:20Z

It's failing because the pull request build doesn't have access to the env var it needs. However, I thought I toggled that to only run when it had the env var. I'll have a look at it.

bethesque · 2021-06-20T22:48:43Z

lib/pact_broker/index/service.rb

+          Sequel.|( *terms.map { |term| Sequel.ilike(Sequel[:consumers][:name], "%#{term}%") })
+        )
+
+        PactBroker::Pacts::PactPublication


I'm still confused as to why you're querying the pact publication table, instead of just querying the pacticipant table directly. To do this query requires that you join the pacticipant table twice to the pact publication table, and then filter on the entire dataset based on two columns (there could be 1000s of pact publications).

If you query the pacticipant table directly, you only do the filtering once, there are no joins, and it's there are likely to be 10s of pacticipants maximum.

I feel like we're not on the same page here. Is there something I'm missing? Shall we have a lunch time catch up?

bethesque · 2021-06-20T23:00:49Z

I've updated the code that runs the pact verify to check for empty string as well as nil. Merge master in, and see if that fixes the Sqlite build.

bethesque · 2021-06-21T23:00:27Z

Just doing some local testing. When I put in "bar" it doesn't find the "bar-provider"

bangn · 2021-06-22T01:53:23Z

Oh I have not tokenise by spliting - 🤦
Probably split by -, / and :space:?

bethesque · 2021-06-22T03:01:01Z

There was no dash in the search string. I just put in "bar". "bar" should match "bar-provider".

bethesque · 2021-06-22T03:02:55Z

The unit test for search indicates that it should work, so I'm guessing the issue is somewhere outside that.

bangn · 2021-06-22T09:02:50Z

Somehow this query

          base.where(Sequel.|(
            Sequel[:pact_publications][:consumer_id] => pacticipant_ids,
            Sequel[:pact_publications][:provider_id] => pacticipant_ids
          ))

return an AND operator instead of an OR

SELECT `pact_publications`.`id` FROM `pact_publications`
WHERE ((`pact_publications`.`consumer_id` IN (1, 3)) AND (`pact_publications`.`provider_id` IN (1, 3)))

I was expecting Sequel.| should return an OR as in https://sequel.jeremyevans.net/rdoc/files/doc/querying_rdoc.html#label-SQL-3A-3AExpression

Sequel.or works tho. Am I missing something here?

bangn · 2021-06-22T09:09:44Z

Found the issue
This code works

          base.where(Sequel.|(
            { Sequel[:pact_publications][:consumer_id] => pacticipant_ids },
            { Sequel[:pact_publications][:provider_id] => pacticipant_ids }
          ))

bangn · 2021-06-22T09:11:04Z

Fixed in c45f3e4

bethesque · 2021-06-23T23:00:08Z

🎉 Awesome! The search results are showing the expected results now.

Next steps:

Keep the search text displayed in the Pacticipant name box after submission (it disappears)
Allow the user to clear the search and view all pacticipants again.
Add the search form to the "tags" view.

bangn · 2021-06-24T17:44:46Z

Next steps:
Keep the search text displayed in the Pacticipant name box after submission (it disappears)
Allow the user to clear the search and view all pacticipants again.
Add the search form to the "tags" view.

Keep the search text displayed in the Pacticipant name box after submission (it disappears)

Done

I'll work on other items and will keep you posted :)

bangn · 2021-06-29T13:14:59Z

Allow the user to clear the search and view all pacticipants again.
Add the search form to the "tags" view.

Done

Used jQuery to redirect to the correct page in this case. I feel a bit hacky tho. Please let me know if you have a better idea

bangn · 2021-07-05T04:24:16Z

Hi @bethesque do you have any feedbacks on this? :D

bethesque · 2021-07-05T05:26:25Z

Almost there!

Let's rename "Pacticipant name" to "Search", because it's not just one pacticipant name, it searches across both consumer and provider.
Can you move the search and reset buttons to the right of the text box, so it doesn't take up so much vertical space.
When I go to the tags index page, and then put in search text and press enter, it takes me back to the no-tags index page. Can you make the enter key work the same way as pressing the search button.

bangn · 2021-07-05T13:09:52Z

Latest view of the form and functionality :D
Please let me know if you have any feedback

test-with-show-and-show-with-tags.mp4

bethesque · 2021-07-06T00:32:01Z

Fantastic! Love it. Thank you so much.

bangn commented Jun 4, 2021

View reviewed changes

bangn force-pushed the issue-318 branch from f5e9e7e to fcfea27 Compare June 4, 2021 15:24

bangn commented Jun 4, 2021

View reviewed changes

bangn force-pushed the issue-318 branch 3 times, most recently from a14a146 to d103459 Compare June 7, 2021 02:07

bethesque reviewed Jun 9, 2021

View reviewed changes

bangn force-pushed the issue-318 branch 2 times, most recently from 94ca4e2 to 84dff5e Compare June 11, 2021 20:48

bethesque reviewed Jun 14, 2021

View reviewed changes

bangn force-pushed the issue-318 branch 2 times, most recently from 07bf41c to 6a7345d Compare June 17, 2021 12:30

bangn force-pushed the issue-318 branch 2 times, most recently from 313d324 to 80388e9 Compare June 17, 2021 13:03

bethesque reviewed Jun 20, 2021

View reviewed changes

bangn force-pushed the issue-318 branch 3 times, most recently from 2538ec1 to d48df4f Compare June 21, 2021 14:03

bangn force-pushed the issue-318 branch from d48df4f to c45f3e4 Compare June 22, 2021 09:10

bangn force-pushed the issue-318 branch from 5000fd1 to 9402336 Compare June 29, 2021 13:21

feat: Filter pacticipant name on index page

08ca2b7

bangn force-pushed the issue-318 branch from 8c7ab8a to 08ca2b7 Compare July 5, 2021 13:08

bethesque merged commit fd882da into pact-foundation:master Jul 6, 2021

bangn deleted the issue-318 branch July 12, 2021 12:22

feat: Filter pacticipant name on index page #446

feat: Filter pacticipant name on index page #446

Conversation

bangn commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangn commented Jun 7, 2021

bethesque commented Jun 8, 2021

bethesque commented Jun 8, 2021

bethesque commented Jun 8, 2021

bangn commented Jun 8, 2021 • edited Loading

bethesque commented Jun 9, 2021

bethesque commented Jun 9, 2021 • edited Loading

bethesque Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bethesque Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

bangn Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

bangn commented Jun 17, 2021

bethesque commented Jun 20, 2021

Choose a reason for hiding this comment

bethesque commented Jun 20, 2021

bethesque commented Jun 21, 2021

bangn commented Jun 22, 2021

bethesque commented Jun 22, 2021

bethesque commented Jun 22, 2021

bangn commented Jun 22, 2021

bangn commented Jun 22, 2021

bangn commented Jun 22, 2021

bethesque commented Jun 23, 2021

bangn commented Jun 24, 2021

bangn commented Jun 29, 2021

bangn commented Jul 5, 2021

bethesque commented Jul 5, 2021 • edited Loading

bangn commented Jul 5, 2021

bethesque commented Jul 6, 2021

bangn commented Jun 8, 2021 •

edited

Loading

bethesque commented Jun 9, 2021 •

edited

Loading

bethesque Jun 9, 2021 •

edited

Loading

bethesque Jun 16, 2021 •

edited

Loading

bangn Jun 17, 2021 •

edited

Loading

bethesque commented Jul 5, 2021 •

edited

Loading