Add course with dept

[charlesconnell]

I'm creating a pull request for Bryan's feature branch so we have a place to talk about it.

[charlesconnell]

First impressions:

• The add course form is now so long that it can't fit on my screen all at once. This is not a good user experience. Let's see if we can pull some inputs onto the same line or shrink up whitespace.
• School names are prefixed by "School XXXXX:". Users should not see our internal ID numbers.
• I like how selected schools/departments are displayed under the associated text inputs once selected.
• The Save button doesn't seem to flip to enabled mode even if I fill out enough fields. Also, it's still clickable even while in disabled mode.
• Filling out the form and clicking Save appears to create the correct model objects, but shows me an error page saying "ImproperlyConfigured at /. No URL to redirect to. Either provide a url or define a get_absolute_url method on the Model."

[charlesconnell]

I'd be happy to begin writing Selenium tests.

[AndrewMagliozzi]

Thanks @CharlesConnell if you could get it started, I think that's a great project for @William-Bratches to own.

On Thu, Mar 13, 2014 at 12:13 PM, Charles Connell notifications@github.comwrote:

I'd be happy to begin writing Selenium tests.

Reply to this email directly or view it on GitHubhttps://github.com/FinalsClub/karmaworld/pull/358#issuecomment-37552159 .

[btbonval]

@charlesconnell Agreed about the look and feel comments. Not really my thing, but I did make some initial attempts.

The selected inputs being shown is a neat trick of django-ajax-selects. It gives with one hand and takes with the other.

What you're describing with "school: ID" is coming out of django-ajax-selects as well. Basically wherever Django Admin goes to generate that is where djang-ajax-selects goes. You're right, that should be cleaned up. Gotta figure out if thats __repr__ or what in the Models.

It might be possible to move form fields around to share lines, but it should ideally be done without template modification. I'm not sure how to do that since we're using the Bootstrap or whatever where we insert classes like "8-columns-small" (out of 12 columns). I don't want this to go back to manually typing out the full template just so we can add a few styling classes here and there. We might be able to insert the desired classes into the Forms, but that's going to get ugly with specifying widgets. I'm not really sure how to handle that in a good way. I was happy to leave it as-is, since it didn't really change my personal experience.

The save button stopped working exactly correctly some time ago. I remember Jacob and I discussed it during a team meeting fairly recently. It wouldn't turn on when it should, or maybe it turned off when it shouldn't, but then one could click it anyway. I decided not to mess with the SAve button while working on this other stuff, but you're right, it needs to be fixed.

You got a redirect error? Can you tell me the exact circumstances? If you look at the following comment, you'll see how many possible ways there are to use this Form which might react differently (I've updated it since my last email): https://github.com/FinalsClub/karmaworld/issues/294#issuecomment-37505246

[btbonval]

Btw, I'd be super appreciative if you could help write unit tests, @charlesconnell . Wasn't sure if you had time for that. If you don't, I can get them started, maybe finished, and worse case it seems like William has some free time to help convert the bullet point list into code.

[btbonval]

Ohhh I forgot to clean up all the manual AJAX. I think, in this branch, we're no longer using any of the AJAX endpoints in the URLs anymore, the Autocomplete JS (forgot where that is), and autocomplete stubs in views. They have been replaced by django-ajax-selects. Now there is one URL include (whch turns into a bunch of URL endpoints) and a few small classes in the models.

[charlesconnell]

@btbonval Do you know why some of the fields in the form have IDs like "c587f123-943d-434e-93d8-d92c8aa7b09d_text"? Where is this coming from?

[btbonval]

@charlesconnell Yes, those are autogenerated ids. Basically Django needs to communicate IDs between Widgets, but Widgets decide their own IDs at render time.There was no way to communicate IDs when the Widgets made up their own. So I had to generate random IDs if none was supplied in order to know before render-time what their IDs were, which enabled communicating that information between widgets. You can override that with attrs={'id': something} if needed.

Code to get Widget DOM ID: https://github.com/FinalsClub/django-ajax-selects-cascade/blob/master/ajax_select_cascade/fields.py#L53-L55

That code generates a DOM ID if one isn't already there: https://github.com/FinalsClub/django-ajax-selects-cascade/blob/master/ajax_select_cascade/__init__.py#L66-L85

Notice that this is part of django-ajax-selects-cascade. I did write it, but it is meant to be useful for more than just our one Django project. I've extended the functionality of django-ajax-selects (which I did not write).

[btbonval]

Oh, I made it easier to manually supply a DOM ID without needing to muck with a Widget: https://github.com/FinalsClub/django-ajax-selects-cascade/blob/master/ajax_select_cascade/fields.py#L33-L35

[btbonval]

Additionally, the "_text" in "whatever_text" comes from django-ajax-selects. The "_text" field is the user's text. The same field sans "_text" is where the autocompleted data object goes. That is code I did not write. I did write the code that makes the "whatever" be a randomly generated UUID if not otherwise supplied at Field/Widget creation.

[charlesconnell]

Okay, makes sense.

[charlesconnell]

I've adapted the 3 existing tests to the new DOM structure. None of them will pass. I have not yet been able to submit the form and see the expected behavior. Sometimes I get the ImproperlyConfigured thing I mentioned above, and sometimes it just reloads the homepage. While I said before that objects were still being created, I cannot get that to happen again.

[btbonval]

Again, this is a fairly complex system. If you could tell me exactly how you're using the form, I can try it out on my system.

Since you probably don't have any Department or Professor objects, I have to assume you're creating a new department by typing in a name and creating a new prof by typing in an email and a name. School must always be dropdown selected (I assume you're doing that). Course name must be something (I assume you're typing something). Robot field empty. Are you using a Course URL?

The specifics are important so I can trace code and see what's going on. If you look at the possible suite of test scenarios, you'll see why I didn't even try to test all of them. I tested one or two scenarios and they worked.

[btbonval]

Also, dumb question I know because you know what you're doing, but did you run the migrations?

[btbonval]

uhm weird. I just got an Import error so I can't even start running. My git repo got outta sync or something.

[btbonval]

Yeah. That branch is trying to import something it that wasn't there. You should not have been able to load the Form at all. It looks like a merge went bad. I'll submit a fix shortly.

Side note to self: import_ocw_json needs to be updated. I should make a ticket for this. (Edit: #359)

[btbonval]

Okay, after the above commit, I tested the feature_course_add_with_dept branch. School: Macomb Community College (selected by dropdown) Department: The Newest Test (typed in, not selected) Professor Name: New Prof (typed in, not selected) Professor Email: np@mcc.edu (typed in, not selected) Do not fill in this field: (I didn't!) Course URL: (left blank)

Course was created and displayed correctly.

Let me know if you're still having problems after pulling the above commit.

[charlesconnell]

@btbonval Looks like that helped. Which fields are supposed to be required? I think there's a descrepancy between what is required according to courses/forms.py and what is actually needed to get the form to pass.

[charlesconnell]

Never mind, my mistake.

[charlesconnell]

Okay we now have 2 tests passing. Current failure is on creating a course, going back to the homepage, and then trying to create another course with exactly the same information. This is resulting in 2 courses, which is not the previous expected behavior.

[btbonval]

Good find. I had not tried that. I have to run some errands today but I'll look into it this evening. On Mar 14, 2014 10:49 AM, "Charles Connell" notifications@github.com wrote:

Okay we now have 2 tests passing. Current failure is on creating a course, going back to the homepage, and then trying to create another course with exactly the same information. This is resulting in 2 courses, which is not the previous expected behavior.

Reply to this email directly or view it on GitHubhttps://github.com/FinalsClub/karmaworld/pull/358#issuecomment-37654745 .

[charlesconnell]

Wrote a system in d177fed for doing tests of every combination of inputs as you suggested @btbonval. Took out the previous test I mentioned until I can find a way to wedge it into this new system, but I plan to bring it back.

[AndrewMagliozzi]

Good catch

On Mar 16, 2014, at 1:52 PM, Charles Connell notifications@github.com wrote:

It looks like professor objects can be created even if the honeypot field is filled in.

— Reply to this email directly or view it on GitHub.

[charlesconnell]

Removed that last comment, it was incorrect.

[btbonval]

Wow Charles, I would have just done tuples of (choice, result) but the FieldAction class is really elegant. It's so much nicer than I would have done. This should make it really easy to add and remove test cases to the Add Course button as its use case changes. For example, each course can support multiple professors, and there's an AutoCompleteSelectMultipleField which will collect a bunch of autocompleting fields for a form. These tests should be pretty easy to modify to support testing multiple professors whenever it gets written.

If I'm reading this correctly, the only way this could possibly improved would be if each option in field_options_combinations created a new class method test_autofill_1, test_autofill_2, etc. It would be sort of hacky (like demon summoning Pythonic dark magic) to dynamically generate unit test test cases as such. Right now all tests are part of the same singular test case, so if one fails, the others don't get tested. If you're really bored (doubt it) and want a challenge, you could try to for loop over field_options_combinations in init generating a suite of new methods that call self.action_test_and_check. I would add some name string to each FieldAction and then name the methods test_autofill_fieldactionnamefieldactionname... concatenating the unique names that are read from each FieldAction in the test. That way if one test fails, the others proceed.

I think about doing this sort of thing every time I write Python unit tests, but I never actually do it. It might not work.

On Sun, Mar 16, 2014 at 2:12 PM, Charles Connell notifications@github.comwrote:

Removed that last comment, it was incorrect.

Reply to this email directly or view it on GitHubhttps://github.com/FinalsClub/karmaworld/pull/358#issuecomment-37764938 .

[charlesconnell]

Did what you suggested @btbonval to split each case into a separate test case. I'm going to take a break from this for now, but it still needs work. It seems like tests are passing that should be failing.

[charlesconnell]

Branch is ready for merge.

[charlesconnell]

Seeing this error when trying to use the form on beta:

ERROR:django.request:Internal Server Error: /
Traceback (most recent call last):
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 115, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/base.py", line 68, in view
    return self.dispatch(request, *args, **kwargs)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/base.py", line 86, in dispatch
    return handler(request, *args, **kwargs)
  File "/var/www/karmaworld/karmaworld/apps/courses/views.py", line 42, in post
    ret = CourseAddFormView.as_view()(request, *args, **kwargs)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/base.py", line 68, in view
    return self.dispatch(request, *args, **kwargs)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/base.py", line 86, in dispatch
    return handler(request, *args, **kwargs)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/edit.py", line 199, in post
    return super(BaseCreateView, self).post(request, *args, **kwargs)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/edit.py", line 165, in post
    return self.form_valid(form)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/edit.py", line 127, in form_valid
    self.object = form.save()
  File "/var/www/karmaworld/karmaworld/utils/forms.py", line 192, in save
    getattr(instance, attribute).add(modelform.save())
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/forms/models.py", line 370, in save
    fail_message, commit, construct=False)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/forms/models.py", line 87, in save_instance
    instance.save()
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/base.py", line 546, in save
    force_update=force_update, update_fields=update_fields)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/base.py", line 650, in save_base
    result = manager._insert([self], fields=fields, return_id=update_pk, using=using, raw=raw)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/manager.py", line 215, in _insert
    return insert_query(self.model, objs, fields, **kwargs)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/query.py", line 1675, in insert_query
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 943, in execute_sql
    cursor.execute(sql, params)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/backends/postgresql_psycopg2/base.py", line 56, in execute
    six.reraise(utils.IntegrityError, utils.IntegrityError(*tuple(e.args)), sys.exc_info()[2])
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/backends/postgresql_psycopg2/base.py", line 54, in execute
    return self.cursor.execute(query, args)
IntegrityError: duplicate key value violates unique constraint "courses_professor_name_2106e3e39e8676ed_uniq"
DETAIL:  Key (name, email)=(, ) already exists.

[charlesconnell]

We merged this branch into master and updated beta. The form was having issues (see Andrew's comment), so we removed the merge commit. So despite what this PR says, this branch is not merged.

[btbonval]

Is this error on beta or prod?

There is a duplicate key error coming out of the database for professor name/email.

There are two problems demonstrated: 1. It tried to create a blank professor name and blank email (it shouldn't do that), 2. It tried to "create" an existing professor entry even though the entry existed when it should have used the existing one.

So there's something wrong with the professorform object creation on the systen. On Mar 18, 2014 6:04 PM, "Charles Connell" notifications@github.com wrote:

Seeing this error when trying to use the form on beta:

ERROR:django.request:Internal Server Error: / Traceback (most recent call last): File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 115, in get_response response = callback(request, _callback_args, _callback_kwargs) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/base.py", line 68, in view return self.dispatch(request, _args, _kwargs) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/base.py", line 86, in dispatch return handler(request, _args, _kwargs) File "/var/www/karmaworld/karmaworld/apps/courses/views.py", line 42, in post ret = CourseAddFormView.as_view()(request, _args, _kwargs) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/base.py", line 68, in view return self.dispatch(request, _args, _kwargs) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/base.py", line 86, in dispatch return handler(request, _args, _kwargs) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/edit.py", line 199, in post return super(BaseCreateView, self).post(request, _args, _kwargs) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/edit.py", line 165, in post return self.form_valid(form) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/views/generic/edit.py", line 127, in form_valid self.object = form.save() File "/var/www/karmaworld/karmaworld/utils/forms.py", line 192, in save getattr(instance, attribute).add(modelform.save()) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/forms/models.py", line 370, in save fail_message, commit, construct=False) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/forms/models.py", line 87, in save_instance instance.save() File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/base.py", line 546, in save force_update=force_update, update_fields=update_fields) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/base.py", line 650, in save_base result = manager._insert([self], fields=fields, return_id=update_pk, using=using, raw=raw) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/manager.py", line 215, in _insert return insert_query(self.model, objs, fields, _kwargs) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/query.py", line 1675, in insert_query return query.get_compiler(using=using).execute_sql(return_id) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 943, in execute_sql cursor.execute(sql, params) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/backends/postgresql_psycopg2/base.py", line 56, in execute six.reraise(utils.IntegrityError, utils.IntegrityError(_tuple(e.args)), sys.exc_info()[2]) File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/backends/postgresql_psycopg2/base.py", line 54, in execute return self.cursor.execute(query, args) IntegrityError: duplicate key value violates unique constraint "courses_professor_name_2106e3e39e8676ed_uniq" DETAIL: Key (name, email)=(, ) already exists.

Reply to this email directly or view it on GitHubhttps://github.com/FinalsClub/karmaworld/pull/358#issuecomment-37994765 .

[btbonval]

The comment from March 18 says this branch was not merged even though the ticket says it was merged (a manual unmerge I guess).

I see some of these changes in the current master, so I'm pretty sure this branch has since been merged into master.

Just kind of clarifying sort of, because I really don't remember what happened with this or where it landed finally.