Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Languagepack progress bar fix #4998

Merged
merged 3 commits into from
Mar 17, 2016

Conversation

MCGallaspy
Copy link
Contributor

Summary

Fixes #4955. But while looking in to this I went down a dead end with the updates management command base classes: https://github.com/learningequality/ka-lite/blob/0.16.x/kalite/updates/management/commands/classes.py#L41

As far as I can tell, it's not possible to pass kwargs to a command's constructor using the call_command function (or possibly at all). For instance, it doesn't seem possible to set the process_name, which was the first thing I attempted. Instead changes to the back-end (like in this case changing the management command that's being called) require changes to the front-end. Is this worth a refactor?

TODO

Test incoming.

  • Have tests been written for the new code? If you're fixing a bug, write a regression test (or have a really good reason for not writing one... and I mean really good!)

Issues addressed

Fixes #4955.

It's tied to the management command that backs it --
the UpdateProgressLog model which backs this app creates
a process_name attribute based on the invoked management
command, which in this case changed from languagepackdownload
to retrievecontentpack.
@aronasorman
Copy link
Collaborator

Tested. Working. Merging.

@aronasorman
Copy link
Collaborator

Actually, won't merge yet. Can you also remove this notification, since a restart is no longer needed:

screen shot 2016-03-17 at 11 18 18

@aronasorman
Copy link
Collaborator

Also, are you planning to write tests for this?

@MCGallaspy
Copy link
Contributor Author

Can you also remove this notification, since a restart is no longer needed:

Yes

For tests, I'm hesitant -- the language pack list depends AFAICT on external variables, namely the central server. So it's not super isolated. The rest of the functionality depends on the list. I would like to assert that the download bar appears or something, but if it breaks when the central server is offline or when factors outside of this app change, it's brittle. Your thoughts?

@aronasorman
Copy link
Collaborator

For tests, I'm hesitant -- the language pack list depends AFAICT on external variables, namely the central server.

I concur. If you can think of a way to isolate this functionality with a unit test, then try mocking or using vcr.py. Otherwise, merge at will.

MCGallaspy added a commit that referenced this pull request Mar 17, 2016
@MCGallaspy MCGallaspy merged commit c053305 into learningequality:0.16.x Mar 17, 2016
@MCGallaspy MCGallaspy removed the has PR label Mar 17, 2016
@aronasorman aronasorman deleted the 4955-progress-bar-fix branch March 17, 2016 21:36
@66eli77
Copy link
Contributor

66eli77 commented Mar 17, 2016

I just pulled and make sure the changes are present. But I didn't see the progress bar when downloading language pack (Spain). Did I miss anything? @MCGallaspy @aronasorman

@MCGallaspy
Copy link
Contributor Author

@66eli77 maybe so... worked for me, seems to have worked for @aronasorman. Perhaps you didn't rebuild the javascript bundles?

@66eli77
Copy link
Contributor

66eli77 commented Mar 17, 2016

could it be that I started a language pack download process before the code change, and I stop the server and git pull the changes, and do the download again?

@radinamatic
Copy link
Member

I just pulled latest 0.16 and can confirm that the progress bar is present. @66eli77 try rebuilding as @MCGallaspy mentioned.

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.

4 participants