Skip to content
This repository was archived by the owner on Sep 19, 2018. It is now read-only.

using get_or_create when testing for group membership #273

Merged
merged 4 commits into from
Aug 13, 2014
Merged

using get_or_create when testing for group membership #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
585d27c
using get_or_create when testing for group membership
ialja Aug 6, 2014
fc4f642
add note about test that has to be fixed
ialja Aug 13, 2014
a3f5d14
make sure address updates on paste
ialja Aug 13, 2014
2752f4f
refactoring and making the events.js code more readable
ialja Aug 13, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 26 additions & 25 deletions static/js/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,19 @@ var Codeweek = window.Codeweek || {};
map = new google.maps.Map(document.getElementById('view-event-map'), mapOptions);
}

function updateAddress(new_position) {
function updateCountrySelection(country) {
var choice = document.getElementById('id_country');
selectItemByValue(choice, country);
}

function updateAddress(new_latLng) {
geocoder = new google.maps.Geocoder();
geocoder.geocode({'latLng': new_position}, function (results, status) {
geocoder.geocode({'latLng': new_latLng}, function (results, status) {
if (status === google.maps.GeocoderStatus.OK) {
document.getElementById("autocomplete").value = results[0].formatted_address;
// the last item in the geocoder for latLng results array is the country
var country = results[results.length - 1].address_components[0].short_name
updateCountrySelection(country);
}
});
}
Expand Down Expand Up @@ -107,30 +115,22 @@ var Codeweek = window.Codeweek || {};
}
}

function fillInAddress() {
var i,
component,
choice,
place = autocomplete.getPlace(),
components = place.address_components,
country = null,
output = autocomplete.getPlace().geometry.location,
outputLat = output.lat(),
outputLng = output.lng(),
locLatlng = new google.maps.LatLng(outputLat, outputLng);

document.getElementById("id_geoposition_0").value = outputLat;
document.getElementById("id_geoposition_1").value = outputLng;

createMarker(locLatlng);
for (i = 0; component = components[i]; i = i + 1) {
if (component.types[0] === 'country') {
country = component.short_name;
choice = document.getElementById('id_country');
selectItemByValue(choice, country);
function listenForPastedAddress() {
var address_field = document.getElementById('autocomplete');
address_field.addEventListener('focusout',function () {
var address_value = address_field.value;
if (address_value) {
getAddress(address_value);
}
}
}, true);
}

function fillInAddress() {
// geoLatLng contains the Google Maps geocoded latitude, longitude
var geoLatLng = autocomplete.getPlace().geometry.location;

createMarker(geoLatLng);
updateAddress(geoLatLng);
}

function auto_complete() {
Expand All @@ -155,6 +155,7 @@ var Codeweek = window.Codeweek || {};
var updated_location = results[0].geometry.location;
createMarker(updated_location);
updateLatLng(updated_location.lat(), updated_location.lng());
updateAddress(updated_location);
}
});
}
Expand All @@ -165,7 +166,7 @@ var Codeweek = window.Codeweek || {};
createMap(initialCenter, 4);
auto_complete();
getAddress(address);

listenForPastedAddress();
}

var add = function (address) {
Expand Down
7 changes: 5 additions & 2 deletions web/tests/test_user_processors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,15 @@ def setUp(self):
pub_date=datetime.datetime.now(),
tags=["tag1", "tag2"])

@pytest.mark.xfail
def test_get_ambassadors_for_country(self):
self.up1.country = "SI"
self.up1.save()

group = Group.objects.get(name="ambassadors")
# temporary workaround because South migrations don't work yet
# fix me in the future
group, created = Group.objects.get_or_create(name="ambassadors")
if created:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary, since get_or_create saves the created object already.
https://docs.djangoproject.com/en/dev/ref/models/querysets/#get-or-create
Not sure what you felt that this has to be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

just an additional test to ensure that the underlying layer actually saves the object

Copy link
Contributor

Choose a reason for hiding this comment

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

Second south migration adds 'ambassadors' group permission. There is a bug in py.test and django_pytest that doesn't correctly run south migrations. I'm still looking up for a fix but it's getting a bit tricky.

Copy link
Contributor

Choose a reason for hiding this comment

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

@goranche I don't know. We could then (re)write the tests for all of the Django, if we adopt this mindset. Not to mention, this is a "bolognese" at it's best :).
I am not exactly sure what is the problem with South? Is this needed because the migrations arent runinig? I am intrigued about pytest having this bug, what exactly is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ercchy everything is bolognese to you ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Only the latest version actually runs South migration - pytest-dev/pytest-django#129 . Last week this patch blew up django-taggit and I haven't got time yet to make a reproducible test case for them. I plan to do it on Wednesday.

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh... @ercchy somehow I thought your comment was about another test...
never mind me 👼

(except for the "everything is bolognese to you" 😬 )

group.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this should ever be done in a test...
if I correctly understand what this does, at least ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you feel should not be done, the get_or_create invocation, or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

the group.save()...
you know, the line I commented on :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I have to ask then, why do you think object should not be saved in a test? I am asking, so you can share your knowledge about this stuff... You know us being a learning group and all...

Copy link
Contributor

Choose a reason for hiding this comment

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

because in this case the ambassadors group should be created during migrate (0002_adding_groups_and_setting_permissions.py), but that doesn't happen for tests at the moment (as far as I understand, this is the issue @gandalfar is having)

a workaround in tests that "fix" stuff that should have happened elsewhere is bad practice

Copy link
Contributor

Choose a reason for hiding this comment

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

Well if this is neessasy for now until we figure out the fix for the migration than let's leave it for the refactathlon.

You said it so perfectly :)

Copy link
Contributor

Choose a reason for hiding this comment

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

in that case I recommend a note (in a comment) stating it should be removed later on


group.user_set.add(self.u1)

Expand Down