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

central-develop into 0.17.x #5595

Merged

Conversation

benjaoming
Copy link
Contributor

Summary

These changes are necessary to align the current central server with the current KA Lite release. Some changes to the control_panel app are necessary to keep data export working for many of the larger deployments. More specifically, they have to generate CSV exports in an async environment, which will be done following a change to the central server itself to just use this upcoming 0.17.6.dev release.

This PR flags a few problematic changes that we need to fix for things to work in both the distributed and the central server environments

TODO

If not all TODOs are marked, this PR is considered WIP (work in progress)

  • Has documentation been written/updated?
  • Have you written release notes for the upcoming release?

Reviewer guidance

I would definitely not mind a quick look from @jamalex and @rtibbles if you happen to have time.

Issues addressed

anuragkanungo and others added 30 commits February 4, 2015 12:08
Student should be redirected to login page if he/she gets looged out while doing playlist exercies
…xam_results

Added student_testing/data/407.json
…c_width

Increased the height of exercise name column in exam reports
…orts

Added Student Mastery/Class Mastery CoachReports
rtibbles and others added 14 commits September 24, 2015 16:38
Massage the Popen arguments into windows form
…-central-develop

Update central-develop to 0.16.x
…p-org-id

Add options object to reference org_id from to ZoneSelectView in data…
Final 0.16.3

 - Fixes learningequality#5087 related to certain Linux kernel version labels
 - Few docs changes
Copy link
Contributor Author

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some notes on whether things should really go in.


#exercise-mastery {
font-size: 10px
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a feature lost from 0.16.x - or it might be unwanted in 0.17?

window.location.href = setGetParam(window.location.href, "playlist", selKeys);
}
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a feature lost from 0.16.x - or it might be unwanted in 0.17?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two exercise_master_view changes are from: #3033

@aronasorman noted:

We can revisit the PRs merged into nalanda by looking at the nalanda-bugfixes milestone, and then porting whatever PRs are relevant to the develop branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSS lives here:

https://github.com/learningequality/ka-lite/blob/master/kalite/coachreports/static/css/coachreports/tabular_view.less

I've searched the codebase for occurrences of "exercise_mastery_view", and as there are none, I will assume that the JS file is unused.

# As we can't predict the current unit on the central server, its better to have the value as 0.
# We can't predict the current unit because in some database we have unit_point_reset gift card for unit 101 whereas the current unit is also 101.
# So we can't find current_unit by saying that the first unit that doesn't have the unit_point_reset gift card is current_unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what all this is. Never heard about the "gift card" part nor the purchased_at__gte in the query further up? We don't have a model StoreTransactionLog anywhere, so I think all of this should go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StoreTransactionLog is a model from kalite.store which is an application we removed.

@@ -14,6 +14,9 @@ require("browsernizr/test/canvas");
require("browsernizr/test/touchevents");
var Modernizr = require("browsernizr");

// Expose this as a global object for use in central server inline JS.
global.getCookie = require("utils/get_cookie");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be configurable and only switched on in the central server.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this might actually be harmless to the rest of the KA Lite application, wouldn't you think so @rtibbles ?

# kalitectl.py wil look for centralserver.settings instead of
# kalite.settings.
new_env = os.environ.copy()
new_env["DJANGO_SETTINGS_MODULE"] = kwargs.get("settings") or "kalite.settings"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should ideally be configurable and only switched on in the central server. The whole function is only used in a test, though, centralserver/central/tests/utils/distributed_server_factory.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all this code is used from the central server's test suite, I'm moving it there.

msg = "\n\n".join([request.body, client_device._hashable_representation(), str(client_device.validate()), client_device.signed_by_id, client_device.id, str(request)])
import inspect
d = client_device
msg = "\n\n".join([str(msg) for msg in [request.body, d._hashable_representation(), d.__dict__, d.__class__, inspect.getfile(d.__class__), local_version, d.description, d.signed_by_id, d.id, request]])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like debugging noise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverting back to the former debug email.

@benjaoming benjaoming force-pushed the central-develop-into-0.17.x branch from 748bc61 to e554397 Compare July 16, 2019 14:35
@@ -61,7 +61,7 @@ def handle(self, *args, **options):
csrftoken = s.cookies['csrftoken_central']
login_data = dict(username=options["username"], password=options["password"], csrfmiddlewaretoken=csrftoken, next='/')
r = s.post(login_url, data=login_data, headers={"Referer": login_url})
assert r.status_code == 200, "Error logging into central server: " + r.content
assert r.status_code == 200 and "Incorrect user" not in r.content, "Error logging into central server: " + r.content
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks like a desired precision from the commit msg: 34a27be

@benjaoming benjaoming force-pushed the central-develop-into-0.17.x branch from 0900e60 to bc367a0 Compare September 2, 2019 16:08
else:
parsed_url = urllib2.urlparse.urlparse(host)
self.url = "%s://%s" % (self.parsed_url.scheme, self.parsed_url.netloc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the latter used to call urllib2.urlparse.urlparse, it seems that somehow it checks if the host is a valid URL with a scheme even though the kwarg was called host.. I don't quite grasp this. The central-develop branch had this, which would cause a regression on 0.17.x:

    def __init__(self, host=None, require_trusted=True, verbose=True):
        self.url = host or settings.CENTRAL_SERVER_URL
        self.parsed_url = urllib2.urlparse.urlparse(self.url)

@benjaoming
Copy link
Contributor Author

Having tested this to the point of registration and by introspecting settings, it seems that the securesync client connects correctly. I had another issues fixed in #5597 - but this should be fine.

@benjaoming benjaoming merged commit fe802df into learningequality:0.17.x Sep 3, 2019
@benjaoming benjaoming changed the title central-develop into 0.17.x [WIP] central-develop into 0.17.x Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants