search

wmde / mitmachen

Help new Wikipedia editors find articles with known issues
https://tools.wmflabs.org/mitmachen
GNU General Public License v2.0
2 stars 1 forks source link

Code Review #49

Closed tobijat closed 4 years ago

tobijat commented 4 years ago

@dev-ckln @darionewmonday @NewMonday Here you can find the code review including all comments: https://github.com/wmde/mitmachen/pull/38 and https://github.com/wmde/mitmachen/pull/38/files

tobijat commented 4 years ago

@dev-ckln If you're fixing the issues from the code review, can you reply to the comments of thiemowmde and resolve the conversation, so we know what has been done and what's still open.

Thanks!

ChristineDomgoergen commented 4 years ago

@dev-ckln @darionewmonday @tobijat are the suggested changes implemented yet?

dev-ckln commented 4 years ago

It is done @ChristineDomgoergen @darionewmonday

ChristineDomgoergen commented 4 years ago

@tobijat can you have a look? Thank you!

tobijat commented 4 years ago

@dev-ckln could you please reply to and/or resolve the conversations in https://github.com/wmde/mitmachen/pull/38/files for the things you have done.

dev-ckln commented 4 years ago

@tobijat @darionewmonday @ChristineDomgoergen Please check the comments:

https://github.com/wmde/mitmachen/pull/38/files#r339067484 The limit of 10000 is there

https://github.com/wmde/mitmachen/pull/38/files#r339069387 Same as above

https://github.com/wmde/mitmachen/pull/38/files#r339070018 What we're getting from current query is title, count based on date means how many categories title are linked in the timestamp.

https://github.com/wmde/mitmachen/pull/38/files#r339076847 I have reduced the size of blacklist file to half of what it was before. Duplicates have been removed from it. Its not loaded again and again, its loaded once the application starts and then it stays in a variable.

https://github.com/wmde/mitmachen/pull/38/files#r339072486 To have some randomness in data and give a sample.

https://github.com/wmde/mitmachen/pull/38/files#r339083851 Done

https://github.com/wmde/mitmachen/pull/38/files#r339079938 jquery ui is used for autocomplete for search.

tobijat commented 4 years ago

@dev-ckln thanks! Here's the second round of code review done by @thiemowmde. The review comments are for this diff: https://github.com/wmde/mitmachen/compare/5e3750a..f6d000e

I grouped the issues into topics. I understand it, that there are no resources left to further work on fixing those issues. However, whenever more work goes into the tool, these issues need to be fixed. Some of them (especially not following best practices and missing documentation) make it hard for anyone to maintain the tool. In terms of severity of the issues, you can read them from the top (more severe) to bottom (less severe).

Code quality issues:

Missing documentation:

Code style issues:

Design issues:

darionewmonday commented 4 years ago

Hi Tobi,

I´m a bit surprised about this new wave of review since it was not expected. As you have mentioned, there are no more resources available to do basically anything which is not working bug. We´re investing on our own side to give you a solution for #17, more than this we can not do.

I have checked the tool on mobile and listed 4 points in 1 new ticket Issue

56.

Tnx Dario

On Wed, Nov 20, 2019 at 3:19 PM Tobi Gritschacher notifications@github.com wrote:

@dev-ckln https://github.com/dev-ckln thanks! Here's the second round of code review done by @thiemowmde https://github.com/thiemowmde. The review comments are for this diff: https://github.com/wmde/mitmachen/compare/5e3750a..f6d000e

I grouped the issues into topics. I understand it, that there are no resources left to further work on fixing those issues. However, whenever more work goes into the tool, these issues need to be fixed. Some of them (especially not following best practices and missing documentation) make it hard for anyone to maintain the tool. In terms of severity of the issues, you can read them from the top (more severe) to bottom (less severe).

Code quality issues:

  • It appears the code contains a lot of repetition now. E.g. there are many checks for the types popular as well as categ. Two very big chunks of code in the main file mitmachen.js are duplicated, with the main difference being some selectors. Can you please extract this code into small, dedicated named functions and avoid the repetition? Thanks.
  • The database name is still hardcoded in multiple places, and not in a config file.
  • Some messages are hard-coded and can not be localized, e.g. multiple "und" in the code.

Missing documentation:

  • I still can't find documentation or code that describes how the blacklist is created. That's not good. Please document this, e.g. next to the code that loads the blacklist.
  • I do see a few suspicious setTimeout with 1000 ms. What are they for? Why 1 second? Can you please add a comment to each of these, explaining what it is for, and why the action needs to be delayed?
  • The SQL files still contain some very suspicious LIMIT 10000. Please add comments to these lines explaining why this particular limit is needed.

Code style issues:

  • There is a larger amount of dead code in / / comments in CSS as well as in comments in HTML. I can't find comments explaining what this dead code is for. Please remove it.

Design issues:

  • I noticed the tool is designed for a very specific screen size. For example, the article preview is hardcoded to be 1260px wide. This won't work on smaller screens, especially not on mobile or iPad-like devices. It might also look odd on screens that are much larger. I can't tell if this is an issue, as I'm not the product owner and not familiar with all requirements. @ChristineDomgoergen https://github.com/ChristineDomgoergen @darionewmonday https://github.com/darionewmonday you might know more about this and if that is an issue from product side. Did you test the tool on mobile devices? Is it required to work there?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wmde/mitmachen/issues/49?email_source=notifications&email_token=AM3ICHHE7WADHHL3V2Y3DQLQUVBOPA5CNFSM4JGLGH22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEESDZAQ#issuecomment-556022914, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM3ICHCZDXHRVALVN7RBATLQUVBOPANCNFSM4JGLGH2Q .

--

Mit Freundlichen Grüßen, Dario Zampetti

Product Design & Strategy

M: +49.176.89002563

dario@newmonday-sprint.de newmonday-sprint.de

tobijat commented 4 years ago

@darionewmonday Most of the time code review does not only exist of one single round (only if there were minor, trivial issues). In this second round, Thiemo checked whether the concerns of the first round were addressed and whether any new issues got introduced. Most of the mentioned points from above were already mentioned in the first round or the issues were introduced after the first round of code review had happened.

@dev-ckln One major issue I've missed in my earlier comment but which is one that has high severity:

darionewmonday commented 4 years ago

Hi Tobi,

I understand but still : ) Pls try to understand us as well.

We´ll try to look into the code issue with the fonts.

Best, D

On Wed, Nov 20, 2019 at 5:39 PM Tobi Gritschacher notifications@github.com wrote:

@darionewmonday https://github.com/darionewmonday Most of the time code review does not only exist of one single round (only if there were minor, trivial issues). In this second round, Thiemo checked whether the concerns of the first round were addressed and whether any new issues got introduced. Most of the mentioned points from above were already mentioned in the first round or the issues were introduced after the first round of code review had happened.

@dev-ckln https://github.com/dev-ckln One major issue I've missed in my earlier comment but which is one that has high severity:

  • The website produces a lot of JS errors in the console. Most of them are the browser refusing to load external fonts because of security reasons. Even if these fonts are not needed, the console output looks scary. Some users know about this and look at it. The console doesn't need to be clean, but should not be that scary. Please remove the code that tries to load external fonts, if they aren't needed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wmde/mitmachen/issues/49?email_source=notifications&email_token=AM3ICHE6BN2S4B2SY22UNDDQUVR5ZA5CNFSM4JGLGH22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEESUJ3Q#issuecomment-556090606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM3ICHH35DVNS3KE2XAFOFTQUVR5ZANCNFSM4JGLGH2Q .

--

Mit Freundlichen Grüßen, Dario Zampetti

Product Design & Strategy

M: +49.176.89002563

dario@newmonday-sprint.de newmonday-sprint.de

dev-ckln commented 4 years ago

Please check, it is fixed

tobijat commented 4 years ago

@dev-ckln @darionewmonday Thanks a lot for working on the code review feedback and fixing some of the issues that were raised. I think we're good enough to close this issue, though some points are still open but I'm going to create separate issues for them so we don't forget them when working on the codebase anytime in the future.

For reference, here's what's left (I will create separate tickets):