Page MenuHomePhabricator

Enable staff to merge suggestions
Closed, ResolvedPublic

Assigned To
Authored By
Samwalton9-WMF
Jan 11 2022, 9:28 AM
Referenced Files
F34970875: image.png
Mar 1 2022, 7:41 AM
F34970869: image.png
Mar 1 2022, 7:41 AM
F34969890: image.png
Feb 28 2022, 1:31 PM
F34969863: image.png
Feb 28 2022, 1:06 PM
F34933751: image.png
Jan 28 2022, 6:51 AM
F34933752: image.png
Jan 28 2022, 6:51 AM
F34933748: image.png
Jan 28 2022, 6:49 AM
Tokens
"Like" token, awarded by Amire80.

Description

Users often file new suggestions for partnerships without checking if that suggestion has already been made. It would be helpful if staff could merge these suggestions together, to get an accurate vote count.

We could add a button to each suggestion, visible only to staff, which says 'Merge'. This would take the user to a dialog where they can select another suggestion to merge it with. It would be helpful if the staff member could see the vote count and description of the suggestions so as to best decide which direction to merge.

When merging, the destination suggestion should receive all the (de-duplicated) votes of the merging suggestion, and the old suggestion should be deleted.

Event Timeline

This would be extremely beneficial to users. The first time I visited the list, I didn't realize there were (I think) three other entries for the site I was suggesting. Each of the entries has a fair number of votes, but they're scattered throughout the list. It might also be helpful to auto-sort the list by domain name (strip it down to just the "xyz.co.jp" to make sure, leaving off any subdomains, including the default "www"). This would make it easier to find duplicates and merge them.

hey @Samwalton9 ! I had a small doubt, while merging suggestions, it should be made mandatory that the merged suggestions should have same 'suggested_company_name' or it's website?
This would mean that merging this suggestions can only have a new merged description

it should be made mandatory that the merged suggestions should have same 'suggested_company_name' or it's website?

Good question! I think staff should be able to merge any suggestion into any other suggestion, and it should keep the destination name. Often users submit the same website but with a slightly different name :)

Good question! I think staff should be able to merge any suggestion into any other suggestion, and it should keep the destination name. Often users submit the same website but with a slightly different name :)

I agree, though I think we should use the most likely name people will search for. This is often simply the name at the top of the website, sometimes also at the bottom of the website with a copyright notice. Regardless, I think the name should be as complete as possible, since I've seen some suggestions that have incomplete names.

Hi @Samwalton9 ! Sorry for being inactive since long, I had one doubt

Is it that say 3-4 suggestions are now merged into one, then the earlier suggestions shall be cleared from the database?

Also the coordinator should have freedom to assign a new company_name, a new description as well as a new company_url (which must have same domain across the suggestions that were merged) to the merged suggestion right?

Hi @Paritosh_Kabra. No problem!

Is it that say 3-4 suggestions are now merged into one, then the earlier suggestions shall be cleared from the database?

Yes, each time suggestion A is merged into suggestion B, suggestion A should be deleted after its upvotes have been merged into suggestion B.

Also the coordinator should have freedom to assign a new company_name, a new description as well as a new company_url (which must have same domain across the suggestions that were merged) to the merged suggestion right?

Don't worry about this - we can already change this data in the administrator interface quite easily. Merging A into B should simply keep the details of suggestion B.

which must have same domain across the suggestions that were merged

Don't worry about this either, staff will pay attention to this. Sometimes the URLs will be slightly different for the same website, but we would still want to merge.

Hi @Samwalton9 ! I issued a PR for this new feature! Kindly look into this one :)

image.png (1×1 px, 230 KB)

After I reabsed my code from master, I am receiving the following error :( , do you know any reason for the same? I am accessing the page as a superuser (which is not supposed to have an associated editor), problem persist with any other path I visit,
image.png (1×1 px, 254 KB)
,
image.png (1×1 px, 208 KB)

You need to run migrations again, a recent code change added a new database field :)

Run docker-compose exec twlight /app/bin/virtualenv_migrate.sh and this should go away.

I'll get this queued up for the team's engineers to review, thanks for your PR!

Hey @Samwalton9 ! Thanks for being patient with clearing my doubts. I had two more :') in the tests that I am running.

The suggest/merge path should be reachable only by coordinators, this works perfectly in the browser through UI, but when I am running the tests, see the code snippet below:

class SuggestionMergeViewTests(TestCase):
    @classmethod
    def setUpTestData(cls):
        super().setUpTestData()

        cls.suggestions_merged_into = SuggestionFactory(
            company_url="www.testingMerged1234.com"
        )
        cls.editor = EditorCraftRoom(cls, Terms=True, Coordinator=True)
        cls.restricted_editor = EditorCraftRoom(
            cls, Terms=True, Coordinator=False)
        cls.upvoters_or_authors = [cls.restricted_editor, cls.editor]
        cls.user = UserFactory(editor=cls.editor)

        cls.suggestions_merged_into.author = cls.editor.user
        cls.suggestions_merged_into.upvoted_users.add(cls.editor.user)
        cls.suggestions_merged_into.save()

        cls.suggestion_merge_count = 5
        cls.company_urls = ["www.testing123.com", "www.testingMerge123.com"]
        cls.suggestions_to_merge = [None]*(cls.suggestion_merge_count)
        for i in range(cls.suggestion_merge_count):
            cls.suggestions_to_merge[i] = SuggestionFactory(
                company_url=random.choice(cls.company_urls)
            )
            upvoter_or_author = random.choice(cls.upvoters_or_authors)
            cls.suggestions_to_merge[i].author = upvoter_or_author.user
            cls.suggestions_to_merge[i].upvoted_users.add(
                upvoter_or_author.user)
        # We should mock out any call to messages call in the view, since
        # RequestFactory (unlike Client) doesn't run middleware. If you
        # actually want to test that messages are displayed, use Client(),
        # and stop/restart the patcher.
        cls.message_patcher = patch(
            "TWLight.applications.views.messages.add_message")
        cls.message_patcher.start()

    @classmethod
    def tearDownClass(cls):
        super().tearDownClass()
        cls.message_patcher.stop()

    def test_merge_partner_suggestion_view_post(self):
        """
        Tests that merging a partner suggestion works properly, with upvotes too getting merged properly
        """
        merge_suggestion_url = reverse("suggest-merge")

        merge_suggestion_data = {
            "suggestions_merged_into": self.suggestions_merged_into,
            "suggestions_to_merge": self.suggestions_to_merge
        }
        factory = RequestFactory()
        request = factory.post(merge_suggestion_url, merge_suggestion_data)
        request.user = self.user
        response = SuggestionMergeView.as_view()(request)  # Permission Denied error comes here despite the fact that I set user with Coordinator=True.

        self.assertEqual(response.status_code, 200)
        self.assertContains(
            response, self.suggestions_merged_into["suggested_company_name"])
        self.assertContains(
            response, self.suggestions_merged_into["company_url"]
        )
        self.assertEqual(self.suggestions_merged_into.upvoted_users.count(), 2)

I inherited the SuggestionMergeView from CoordiantorsOnly and FormView. The screenshot below may also help you clarify this doubt

image.png (978×1 px, 123 KB)

The test below this fails as the response does not return success.

Doubt Two: How can i use message_patcher to test for a particular message?

Thanks for bothering such a long comment :')

Hi @Paritosh_Kabra! Try adding the coordinator like this: https://github.com/WikipediaLibrary/TWLight/blob/master/TWLight/resources/tests.py#L614. You can add an extra commit to your PR so I can make a better review.

Thanks @Scardenasmolinar ! That worked!! I will fix some of the tests on the new commit added to the PR in a while and ping if stuck again.

Hi @Samwalton9 !! Just an out of this issue question, is this project going to be a part in the project ideas for gSOC 2022 😅, or where can I see other ideas released by wikimedia ?

Hi @Samwalton9 !! Just an out of this issue question, is this project going to be a part in the project ideas for gSOC 2022 😅, or where can I see other ideas released by wikimedia ?

I don't think this project will be part of GSOC 2022. You can check the rest of Wikimedia's GSoC progress at https://www.mediawiki.org/wiki/Google_Summer_of_Code/2022. It looks like a few projects have been listed already :)

@Paritosh_Kabra Are you still interested in and able to work on this task? If so, just let me us know. If not, I'll make a few changes and merge it in since it's mostly complete.

@Paritosh_Kabra Are you still interested in and able to work on this task? If so, just let me us know. If not, I'll make a few changes and merge it in since it's mostly complete.

Presently I have my university exams up..I will work on this after they get over around 30th april, if it's okay

@Paritosh_Kabra Checking back in since it's been a few weeks: are you still interested in and able to work on this task sometime in the next week? If not, we're happy to get it into a mergable state and get your contributions deployed.

Sorry for not informing, but I won't be able to work on this presently. Thanks for your help and support, I wish to continue working on this project in later time.

@Paritosh_Kabra Checking back in since it's been a few weeks: are you still interested in and able to work on this task sometime in the next week? If not, we're happy to get it into a mergable state and get your contributions deployed.

It's no problem at all, we'll finish this one up. Thanks!

Based on the meeting today, I will add a guard clause to prevent a suggestion to merge itself.

@jsn.sherman, upon further inspection, the code already excludes a secondary suggestion if it's also the main suggestion. Moving this back to Review.

Thanks for the heads-up; it had been long enough since I looked at it that I forgot. Merged!