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.

Sign up for GitHub

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

Apply the double-search change to simple search #208

Merged
merged 1 commit into from
Mar 17, 2024
Merged

Apply the double-search change to simple search #208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions application/libraries/Librivox_search.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,15 @@ function advanced_title_search($params)
// ======================================================================================================================================



// Combine all of our results as a sub-query, so we can count the results in one place.
$sql .= 'WITH results AS (';


//***** Projects from compilations *****//

$cols = array();

$sql .= 'WITH results AS (';

$cols[] = 'st.source_table';
$cols[] = 'st.search_field';
$cols[] = 'p.title';
Expand Down
6 changes: 5 additions & 1 deletion application/libraries/Librivox_simple_search.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ function simple_search($params)
$offset = (int)$params['offset'];
$limit = (int)$params['limit'];

$sql = '';

// Combine all of our results as a sub-query, so we can count the results in one place.
$sql = 'WITH results AS (';


//***** Titles, Sections from compilations *****//
Expand Down Expand Up @@ -324,6 +326,8 @@ function simple_search($params)

//***** finalize query parts *****//

$sql .= ") SELECT *, COUNT(*) OVER() as full_count FROM results";
Copy link
Member

Choose a reason for hiding this comment

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

So hold on, in https://github.com/LibriVox/librivox-catalog/pull/207/files#diff-cb39737e00dff1ebcb3d8fc914228975e384415679c2f9495c369c537b65318cR183 you changed the code to use the full_count directly in the search results. In that same PR, you modified advanced_title_search() to return the full count, but you didn't touch simple_search(). You're making simple_search() return the full_count in this PR, so my question is - how does simple search even work currently now? It seems to work, but I'm not sure I understand how.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since that first PR was merged, simple search does not work on current master. It wants that column, but doesn't get it.

Since it is working in prod right now, it must still be running a version before that PR.

Copy link
Member

Choose a reason for hiding this comment

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

I git pulled in prod when I merged that PR. Very curious...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just tested again: current master still fails on simplesearch, giving an HTML error message rather than the JSON array the page expects.

I don't know how that git pull could have failed, but I'm glad it didn't take, until this one is in. We did say simplesearch needed patched. 😬

Copy link
Member

Choose a reason for hiding this comment

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

I just tested again: current master still fails on simplesearch, giving an HTML error message rather than the JSON array the page expects.

Can you link an example? I'm legit trying every search that I know of, and they all appear to be working.

I don't know how that git pull could have failed, but I'm glad it didn't take, until this one is in. We did say simplesearch needed patched. 😬

You did, and I think this is a case of my day job expectations and assumptions leaking into LibriVox. In my day job, master is always working, and we never knowingly merge broken or incomplete code. In my mind this is a shared understanding that everyone else, including yourself, has as well, in all projects. That's obviously not the case.

I would strongly prefer if we made such a policy explicit for LibriVox: never knowingly merge broken or incomplete code, even if we don't have the CI coverage needed to automatically enforce that. But this is something that we can discuss.

Copy link
Collaborator Author

@redrun45 redrun45 Mar 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Edit: In case you saw the earlier version, this has been edited. For some reason, it's not showing the 'Edited' tag.

Can you link an example? I'm legit trying every search that I know of, and they all appear to be working.

That would seem to be because master isn't what's in prod. With my dev instance on librivox/master, I get nothing but a spinning wheel.

In my day job, master is always working, and we never knowingly merge broken or incomplete code.

I share the same expectation: nothing that isn't working should actually make it in to master. I just didn't think I was merging the incomplete code. I think I see the disconnect.
The PR was tagged with "[Sanity Check]", which you recognized as meaning that it needed some feedback. I should have also been more explicit (as Gareth was in his comment) that it was not complete even if it was the right technical direction.

When you asked if I had "any objections to merging this", I thought you referred to the PR in general, which I thought we all knew was not complete and needed a second commit. So I said:

if the technical direction is sound, then no, I'm not opposed. I just have to patch the simplesearch part too. I'll get a second commit ready soon!

Perhaps I should have instead said:

if the technical direction is sound, then I'll get simplesearch fixed per Gareth's comment. I'll remove the "[Sanity Check]" tag when I've got it all ready and tested. I expect to have the second commit ready soon!

We can also talk through email if you prefer.

Copy link
Member

@notartom notartom Mar 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Edit: In case you saw the earlier version, this has been edited. For some reason, it's not showing the 'Edited' tag.

Can you link an example? I'm legit trying every search that I know of, and they all appear to be working.

That would seem to be because master isn't what's in prod. With my dev instance on librivox/master, I get nothing but a spinning wheel.

Prod is definitely on latest master, hence my asking for an example or URL where I can hopefully see simple search failing, if only on staging or localdev.

In my day job, master is always working, and we never knowingly merge broken or incomplete code.

I share the same expectation: nothing that isn't working should actually make it in to master. I just didn't think I was merging the incomplete code. I think I see the disconnect. The PR was tagged with "[Sanity Check]", which you recognized as meaning that it needed some feedback. I should have also been more explicit (as Gareth was in his comment) that it was not complete even if it was the right technical direction.

When you asked if I had "any objections to merging this", I thought you referred to the PR in general, which I thought we all knew was not complete and needed a second commit. So I said:

if the technical direction is sound, then no, I'm not opposed. I just have to patch the simplesearch part too. I'll get a second commit ready soon!

Perhaps I should have instead said:

if the technical direction is sound, then I'll get simplesearch fixed per Gareth's comment. I'll remove the "[Sanity Check]" tag when I've got it all ready and tested. I expect to have the second commit ready soon!

I'm used to the "[WIP]" (Work in Progress) or "[DNM]" (Do Not Merge) tags in the commit message title. DNM is much stronger and indicates trying to debug something, often with the CI system itself. The expected outcome of a DNM patch is for it to be abandoned. WIP indicates that the patch isn't ready to merge yet, but there's an expectation that the WIP tag will get removed, at which point the patch will be ready to merge. In retrospect, PR #207 should have been a [WIP].

In my day job, because we use Gerrit and a gating CI, there's also the handy vote system, so the patch author can vote "Workflow -1" on their own patch, clearly indicating that they don't want it to merge at this time. We can try something similar here, by requiring approval from yourself (@redrun45) and myself on all patches (and potentially @garethsime if they're willing to accept the responsibility?). That way, if you're proposing a PR that isn't ready yet, you can disapprove your own PR, and then change your approval once it's ready.


$sql .= ($sort_order == 'catalog_date')
? ' ORDER BY FIELD(blade, "title", "group", "reader", "author") , 6 DESC '
: ' ORDER BY FIELD(blade, "title", "group", "reader", "author") , 4 ASC ';
Expand Down
Loading