search

commons-app / commonsmisc

4 stars 5 forks source link

Merge db call for log title and page #11

Closed maskaravivek closed 5 years ago

maskaravivek commented 5 years ago

Fixes #10

Haven't been able to test the query.

urbanecm commented 5 years ago

Looks the query works

` MariaDB [commonswiki_p]> select log_title, log_page from logging_userindex where log_type="upload" and log_user=(select user_id from user where user_name="Ainali"); +--------------------------------------+----------+ | log_title | log_page | +--------------------------------------+----------+ | Domkyrkan_Karlstad.JPG | NULL | | Sola_i_Karlstad.JPG | NULL | | Karl_IX_Staty_Karlstad.JPG | NULL | | Gustaf_Fröding_Staty_i_Karlstad.JPG | NULL | | Nils_Ferlin_Staty_Karlstad.JPG | NULL | | Unionsupplösningen_staty.JPG | NULL | | Hallsbergs_tågstation.JPG | NULL | | Matthiaskyrkan_Budapest.JPG | NULL | | TerrorHaza_Budapest.JPG | NULL | | Brasov_Rumänien.JPG | NULL | (...) +--------------------------------------+----------+ 4825 rows in set (0.80 sec)

MariaDB [commonswiki_p]>

`

There are also rows with log_page != NULL. As you wrote this patch, I'd like to ask you if this is expected or not.

maskaravivek commented 5 years ago

Yes, its possible that log_title is present but log_page is set to null. For this particular user, there are 3588 records with non-null log_page.

Have added a check on line number: feedback.py:83 so that the sql in query is appended only if log_page is not null.

@urbanecm you can test the PR now. Have rebased the PR. The queries should work properly.

Instead of calling this query 4 times we are now calling it once and reusing the results.

urbanecm commented 5 years ago

Looks good.

urbanecm commented 5 years ago

I've reverted my merge, as I made a terrible mistake while testing, as I forgot to checkout the version from master - which worked.

Looks the real version after this PR doesn't return any valid JSON, it returns "Content-type: application/json" literally. For testing purposes, I've checkouted maskaravivek/commonsmisc as-of ce45286b983dbf7db611e43fdde7e03adad736b3 to https://tools.wmflabs.org/urbanecmbot/commonsmisc/maskaravivek, so https://tools.wmflabs.org/urbanecmbot/commonsmisc/maskaravivek/feedback.py?user=Martin_Urbanec shows the error.

maskaravivek commented 5 years ago

@urbanecm Is it possible for me to push my changes and test it somehow locally or elsewhere?

I am totally dependent on you reviewing my changes, testing it and deploying it. It would be great if you could suggest ways to test it locally or on toolslab.

urbanecm commented 5 years ago

Sorry for not thinking about this before. I've created single-purpose script at https://tools.wmflabs.org/urbanecmbot/commonsmisc/maskaravivek/pull.php. It will issue git pull in your repository's folder. If you want to change branch, you can also use optional branch parameter, so the URL will look like https://tools.wmflabs.org/urbanecmbot/commonsmisc/maskaravivek/pull.php?branch=logging. If the branch parameter is present, it'll execute git checkout branch.

Alternatively, as it looks you already have access to Toolforge, you can clone the repository to public_html directory in any of your tools and then run the webservice by issuing webservice start. Then, you can use https://tools.wmflabs.org/mytool/commonsmisc/feedback.py for testing.

Unfortunately, I'm not aware of any way how to test this script locally. without trying to replicate the toolforge environment by constructing millions of SSH tunnels and domain names in /etc/hosts. I hope this helps.

Let me know if you need anything else from me.

maskaravivek commented 5 years ago

@urbanecm Wow! This would help immensely. I will play around with the queries and see if any of it can be optimized. :)