LB-530: Refactor the LastFm Modal code

[kshitijk83]

Problem

Currently the function for showing messages in LastFm Importer Modal is present in other components. However this is against React's design principles and makes it difficult to add new code or test the existing code. Refactoring the code so that the content shown by Modal is controlled by the component itself will solve the issue.

For reference this is the code which has to be refactored

https://github.com/metabrainz/listenbrainz-server/blob/master/listenbrainz/webserver/static/js/src/LastFMImporterModal.tsx

https://github.com/metabrainz/listenbrainz-server/blob/master/listenbrainz/webserver/static/js/src/Importer.tsx

JIRA ticket: https://tickets.metabrainz.org/browse/LB-530

Solution

Refactored the LastFmImporter.tsx code to include the importing functionality which was delegated to Importer.tsx file, kept the same private variables and initialized them in the constructor function. The workaround for updating state using setInterval wasn't required now (so i removed it) and was performance degrading as setInterval was rerendering the component after each 100ms.

#Action
Do we still need static functions in this component after refactoring?

[brainzbot]

Can one of the admins verify this patch?

[brainzbot]

Can one of the admins verify this patch?

[brainzbot]

Can one of the admins verify this patch?

[brainzbot]

Can one of the admins verify this patch?

[shivam-kapila]

Thanks for the contribution Looks good. Maybe @ishaanshah has some other things in mind too. Would like his review. Other than this please remove the duplicated code from Importer.tsx and not to forget the tests.

[shivam-kapila]

I checked this PR out and saw a lot of linting related issues, especially Must use destructuring props assignment. You can observe these issues (and correct some of them automatically) by running lint.sh. Please look into this too

[shivam-kapila]

this looks good. I think we can remove the redundant code from Importer..tsx and fix the tests (cut paste IG)

[ishaanshah]

Hey, thanks a lot for opening the PR.
I tested PR, however I found out couple of regressions

• The importer doesn't report an error if the user is invalid.
• The importer also doesn't report an error if there's an network error and the page freezes.

Also can you add regression tests for the above errors to ensure that it doesn't happen in the future.

[kshitijk83]

Hey, thanks a lot for opening the PR.
I tested PR, however I found out couple of regressions

* The importer doesn't report an error if the user is invalid.

* The importer also doesn't report an error if there's an network error and the page freezes.

Also can you add regression tests for the above errors to ensure that it doesn't happen in the future.

Hey @ishaanshah .. regarding the page freeze issue, i noticed it is freezing only when the server is not running, i guess that's bound to happen if i stop the server while clicking submit. Can you explain more about this issue, as in my system it is working fine and is not freezing on the network error given that the server is running.

[ishaanshah]

Hey @ishaanshah .. regarding the page freeze issue, i noticed it is freezing only when the server is not running.

Hi, thanks for making the changes. You are correct about the freezing issue, I accidentally disabled all networks (including docker) which made the server unreachable and caused the free. On turning of internet access the importer worked as expected. The PR looks good to me, thanks again for opening it. I'll let @MonkeyDo have a look at it and merge it.

[shivam-kapila]

@kshitijk83 I think there must be some redundant code left in the original files after you moved the functions. Can you remove that please?

[shivam-kapila]

@brainzbot retest this please

[shivam-kapila]

Thanks for your contribution. This looks ready. The failing JS build is failing due to a known other is (fixed in #1228), related to this issue. The changes here are good to go 🎉

[MonkeyDo]

I'm not very familiar with the LastFM importer code, but the PR looks fine to me, thanks for opening it ! :)