Skip to content

Conversation

@shanbady
Copy link
Contributor

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5598

Description (What does it do?)

This PR adds a "display label" field to percolate queries which overrides any other label for display on the frontend

Screenshots (if appropriate):

Screenshot 2024-09-25 at 3 36 49 PM

How can this be tested?

  1. Checkout this branch and run migrations.
  2. Subscribe/follow to a few things
  3. Go to django admin and edit a percolate query you are subscribed to. Put something custom in the "Display label" field
  4. go to your settings page and view your subscriptions list. note that the subscription displays your custom label

@shanbady shanbady marked this pull request as ready for review September 25, 2024 20:00
@shanbady shanbady added the Needs Review An open Pull Request that is ready for review label Sep 25, 2024
@mbertrand mbertrand self-assigned this Sep 26, 2024
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Works great! Made a suggestion for improving the new test. I thought of a few minor nitpicks that might be nice-to-haves but aren't critical and aren't mentioned in the issue:

  • Prepopulate display_label for existing channel-based percolate queries (to channel.title)?
  • When new percolate queries for channels are created, maybe assign the channel title as the initial display_label value
    nvm about the above two, channel title might change
  • Might be nice to add display_label to PercolateQueryAdmin so they can be seen/searched in django admin:

Screenshot 2024-09-26 at 10 38 40 AM

Also left some other inline code comments, but those are probably out of the scope of this PR. Just the test change should be enough for approval.

"certification": None,
"yearly_decay_percent": None,
}
test_label = "new courses about cats"
Copy link
Member

@mbertrand mbertrand Sep 26, 2024

Choose a reason for hiding this comment

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

source_description doesn't seem to be getting tested here, suggest doing something like this:

@pytest.mark.django_db
@pytest.mark.parametrize("test_label", ["new courses about cats", ""])
@pytest.mark.parametrize("is_channel_query", [True, False])
def test_percolate_query_display_labels(
    mocker, mocked_es, test_label, is_channel_query
):
    """
    Test that makes sure we display the display label for a percolate query if it is defined
    """
    mocker.patch(
        "learning_resources_search.indexing_api.index_percolators", autospec=True
    )
    mocker.patch(
        "learning_resources_search.indexing_api._update_document_by_id", autospec=True
    )
    channel = ChannelFactory.create(
        search_filter="department=physics", channel_type="department"
    )

    original_query = {"department": ["physics"]} if is_channel_query else {
        "q": "testing search filter",
        "certification": None,
        "yearly_decay_percent": None,
    }
    query = PercolateQueryFactory.create(
        original_query=original_query,
        query=original_query,
        display_label=test_label,
    )
    assert query.source_description() == (
        test_label
        if test_label
        else channel.title
        if is_channel_query
        else query.original_url_params()
    )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed test

Copy link
Member

Choose a reason for hiding this comment

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

Delete this line, it is overriding the parameterized value:

test_label = "new courses about cats"

Also:

    original_query = {"department": ["physics"]} if is_channel_query else {
        "q": "testing search filter",
        "certification": None,
        "yearly_decay_percent": None,
    }

return self.display_label
if channel:
return channel.title
return self.original_url_params()
Copy link
Member

Choose a reason for hiding this comment

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

Q: Should something else be displayed that's more user friendly than this when display_label and channel are both None?

Screenshot 2024-09-26 at 11 07 19 AM

Copy link
Member

Choose a reason for hiding this comment

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

One thing I found a bit counterintuitive but is outside the scope of this PR: the source_description is used as the title in this list while source_label is shown underneath that. I would expect the opposite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

working on another pr for sending the emails I can see about changing this there.

return "saved_search"

def source_description(self):
channel = self.source_channel()
Copy link
Member

Choose a reason for hiding this comment

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

Outside of this scope's PR changes, but should this function also take the source_type into consideration?

    def source_channel(self):
        original_query_params = self.original_url_params()
        channels_filtered = Channel.objects.filter(search_filter=original_query_params)
        if channels_filtered.exists() and self.source_type ==  CHANNEL_SUBSCRIPTION_TYPE:
            return channels_filtered.first()
        return None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

potentially. - im currently working on another PR that involves sending actual emails for percolated searches so this might change there

@mbertrand mbertrand added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Sep 26, 2024
@shanbady shanbady requested a review from mbertrand September 27, 2024 16:51
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

👍

@shanbady shanbady merged commit 83f502d into main Sep 27, 2024
@shanbady shanbady deleted the shanbady/new-courses-subscription-for-migrated-users branch September 27, 2024 18:53
@odlbot odlbot mentioned this pull request Oct 1, 2024
10 tasks
mbertrand pushed a commit that referenced this pull request Oct 4, 2024
* adding display label and migration to percolate queries

* regenerating specs

* adding test and method for returning label

* fixing migration

* adding display label to admin

* adding test for source description

* fixing search conditionals in test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants