[Issue #3685] save search get opportunity id

[babebe]

Summary

Fixes #{3685}

Time to review: 10 mins

Changes proposed

Query the search index limiting the results to only opportunity ID
Store the list of opportunity IDs returned to the DB as a list/array (does not need to point to opportunity table as foreign keys)
Modify the users query to replace with static pagination

Additional information

Updated tests

[chouinar]

Could you move the definition of the static pagination values to a constant at the top of the file?

A very slightly briefer syntax you can do that avoids needing ** unpacking:

Suggested change

	updated_search_query = {
	**search_query,
	"pagination": {
	"order_by": "post_date",
	"page_offset": 1,
	"page_size": 1000,
	"sort_direction": "descending",
	},
	}
	updated_search_query = search_query | STATIC_PAGINATION

[babebe]

Done

[chouinar]

This entire chunk is duplicated across both this new implementation and the existing one (with just the aggregation flag different), could you move that into some shared bit of code they both use? That way if we need to make changes, it can affect both without missing it.

[babebe]

GP. Created a helper function, done.

[chouinar]

Ideally, we'd have the following in this file:

with db_session.begin():
     saved_search = some_function_call(...)

I want to aim for as little logic as possible outside the service files - that makes the code more reusable (like how the search logic can be reused).

I'd just make create_saved_search have the first thing it does is fetch the opportunity IDs.

[babebe]

Done

[chouinar]

Don't import across unit tests like this - if the setup data has any shared use, move it to the conftest files that are already shared between the unit tests.

Effectively when we're talking test setup, it should only go in one of three places:

• Top of the same test file - for when test setup only pertains to whatever we're testing in that file
• conftest.py in the same directory - when multiple test files in the same directory want to reuse it
• conftest.py in the root tests folder - a lot of tests across the codebase want to use it

[babebe]

I did not think creating common test data in the conftest was appropriate. I created reusable opportunities for the test module.

[chouinar]

I'd recommend putting anything like this into a utility/fixture - this is something we likely want in several unit tests, we shouldn't need to copy-paste chunks of code for every unit test.

We do already have a fixture for setting up the search index alias, but it's at the session level (so across all tests), might need to adjust that to also support other levels.

[babebe]

I did not want to change the existing session scoped fixture, as it is used in other session scoped fixtures. Creating another function scoped fixture looked like an overkill that's why i did this. You think we should create function scope fixture for the opportunity alias ?

[chouinar]

Yeah, a function scoped session would be fine - a module/class level one might also be worth setting up as we usually are fine with test setup for a whole file/class being reused across everything in that file/class (since that's easier to follow / faster running tests).

[babebe]

Done

[chouinar]

@babebe - could you make sure your PR title follows our usual format? Should be: [Issue #XXXX] Some sort of description - these are used in the deploy metadata