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

[issue1021] Add asynchronous page update capability #1359

Merged
merged 5 commits into from Aug 17, 2014

Conversation

Projects
None yet
4 participants
@ehashman
Copy link
Member

commented Aug 17, 2014

Branches off of #1352. By new issue id, addresses #1336.

@coveralls

This comment has been minimized.

Copy link

commented Aug 17, 2014

Coverage Status

Coverage increased (+0.13%) when pulling ab2ed80 on ehashman:issue1021 into f601dfe on openhatch:master.

@coveralls

This comment has been minimized.

Copy link

commented Aug 17, 2014

Coverage Status

Coverage increased (+0.13%) when pulling 24091c3 on ehashman:issue1021 into f601dfe on openhatch:master.

return filter(bool, [x.strip() for x in bugtext.split("\n")])

@staticmethod
def is_valid_url(x):

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

You might, for the lulz, and for posterity, explain why we wrap this core Django thing in our own function.

The answer is that exceptions are a pain, and we prefer nice simple functions that return True or False.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 17, 2014

Author Member

Exceptions = "my poor call stack :'("

def __init__(self, *args, **kwargs):
# FIXME: Our home-baked "ModelForm" could be implemented with django

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

Such +1 on this comment.

return True

if not all([is_valid_url(url) for url in buglist]):
if not all([BugsForm.is_valid_url(url) for url in buglist]):

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

I suppose if we care about properly functioning in the face of inheritance, we should refer to is_valid_url() via self. or something. Shrug.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 17, 2014

Author Member

I'm probably just going to leave this as is; this code wasn't written to be extended.

<h1 style='margin: 10px 0 0 10px; font-weight: bold; color: #222'>
Create a Bug Set
</h1>
<h1>Create a Bug Set</h1>

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

Yay! Go away, inline style.

@@ -131,7 +140,6 @@ def update(self):
url=bug))

for bug in new_bugs - old_bugs:
# get_or_create returns a tuple (object b, bool created?)

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

It's more idiomatic to write:

obj, created = whatever.get_or_create(whatever)

and then throw away "created". Similarly, you could do, like,

# _ to indicate we ignore this
obj, _ = whatever.get_or_create(whatever)

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 17, 2014

Author Member

Yeah, I saw that when looking around for how to deal with this. Wasn't a fan of the idiom but it's good to be consistent. Can definitely clean this up.

This comment has been minimized.

Copy link
@ArcTanSusan

ArcTanSusan Aug 17, 2014

Contributor

Or, even _created.

@@ -21,10 +21,10 @@
Event Bug Sets
{% endblock title %}

{% block body_id %}bugsets_main{% endblock %}

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

Yay.


self.assertEqual(200, response.status_code)
self.assertContains(response, "Create a Bug Set")

def test_create_view_redirect(self):
client = self.login_with_client()

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

In the OpenHatch codebase, we're not consistent about this, but anyway, fwiw, one thing you can do is self.client = self.login_with_client()

In fact, arguably, we should change self.login_with_client() so that it mutates self.client.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 17, 2014

Author Member

You might want to open an issue for that; I'm not sure how important/useful such a change would actually be though.


self.assertEqual(200, response.status_code)
self.assertNotContains(response, self.removed_bug)

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

In general, I'm very happy with this functionality. One thing I'm concerned about is if people manage to confuse themselves by having two different tabs open, with different state.

One thing we could do to ameliorate that is what (iirc) MediaWiki used to do, which was that when the page presents you the form, it adds a hidden field storing the last modification time. If, when you submit the form, the last modification time != the current last modification time in the backend, it refuses to save, and says, "Hey, go fix up any merge conflicts, and try again."

Then to improve usability, MediaWiki started doing a 3-way merge... and things got complicated, so if we don't want to go that route, it's understandable.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 17, 2014

Author Member

One thing I'm concerned about is if people manage to confuse themselves by having two different tabs open, with different state.

django-inplaceedit pulls the current DB value when you edit a field, not the page value, so that kind of edit won't be a concern. And with a 5s update timer, I doubt that will be an issue.

If the concern is that the bug is removed from the set but a user can still edit it from the outdated page... well, that seems like a bit of an edge case, not sure it's much of a concern anyways. That may be something to look into in the future.

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

Yeah -- I mean the latter concern. I agree it's not a show stopper. Mostly I think it'd be cool if we had, like, a code comment somewhere acknowledging this possible UX risk.

b = mysite.bugsets.models.AnnotatedBug.objects.get(
pk=data['obj_id'])

return HttpResponse(json.dumps({

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

In theory, it's important for security for us to serve this out with the right JSON mime type.

I think the right MIME type is application/json.

except Exception:
return HttpResponse(json.dumps({
'error': 'Unknown error',
}))

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

You're calling json.dumps() a few times. Maybe you can refactor this slightly and do that once at the bottom.

Also, for the "Object does not exist!" error, this should probably be a 404, not a response code of "200 OK".


function runOnInterval() {
setTimeout(runOnInterval, 5000); // 5s = 5000ms
updateFields();

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

I like to setTimeout() after my other things, not before them, to avoid horrifying pile-up.

Similarly, in theory, it'd be nice if you can make the setTimeout() call here after the fields have all been updated, but that is definitely going to be more of a pain.

This comment has been minimized.

Copy link
@ehashman

ehashman Aug 17, 2014

Author Member

Oh, whoops, I switched the order for testing and forgot to put it back. Thanks for catching this.

This comment has been minimized.

Copy link
@paulproteus

paulproteus Aug 17, 2014

Contributor

Along these lines, you could have updateHTML be the one to call setTimeout(). But you'd have to make it so that only the last updateHTML() is the one that calls setTimeout().

Kind of a pain, but maybe less of a pain than I thought.

paulproteus added a commit that referenced this pull request Aug 17, 2014

Merge pull request #1359 from ehashman/issue1021
[issue1021] Add asynchronous page update capability

@paulproteus paulproteus merged commit 743344f into openhatch:master Aug 17, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@paulproteus

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2014

I'm happy to merge this as-is, and take any improvements in follow-on pull requests. This work is huge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.