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

Can't change course id in Courses admin #613

Closed
jonespm opened this issue Jul 29, 2019 · 6 comments · Fixed by #1468
Closed

Can't change course id in Courses admin #613

jonespm opened this issue Jul 29, 2019 · 6 comments · Fixed by #1468
Assignees
Labels
⚙️ backend 🪳 bug Something isn't working

Comments

@jonespm
Copy link
Member

jonespm commented Jul 29, 2019

Thank you for contributing to this project!

  • Make sure to search the issues for duplicates first!

Expected behavior (A clear and concise description of what you expected to happen) :

When I change the ID in the courses admin for a course, it should change the ID

Describe the bug (Tell us what happens instead of the expected behavior) :

When I change the ID there is a stack trace. It looks like it creates another course with none of the default view options set with this course ID and keeps the original. The error looks like it's in Django.

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
django.template.base.VariableDoesNotExist: Failed lookup for key [non_field_errors] in 'None'

Steps to Reproduce :

  1. Create a Course as admin in the Django Admin Panel
  2. Change the ID number and hit Save
  3. Note that the original is not updated but a new one is created.

Screenshots/Video (If possible, add media to help explain your problem) :

image

Additional context (Add any other context about the problem here) :


Test Plan:

  1. Courses admin form is working fine
  2. Add a new course through LTI launch and Django Admin console
  3. Update an existing course form from admin view and it works as expected
@aberatne
Copy link

aberatne commented Dec 9, 2019

I noticed that saving the course object after converting canvas id to incremented id will update the course view options after saving the course.

@aberatne
Copy link

aberatne commented Dec 9, 2019

I was trying to override the save_model() function using save(force_update=True). Is it possible to do something like this to ensure that the course instance gets the updated id instead of creating a new instance?

@jennlove-um
Copy link
Contributor

Testing shows that this behavior still occurs in the most recent release.

1 similar comment
@jennlove-um
Copy link
Contributor

Testing shows that this behavior still occurs in the most recent release.

@ssciolla
Copy link
Contributor

ssciolla commented Jan 9, 2023

In my initial testing, I did not see the same stack trace that @jonespm highlighted in the initial issue, but I did see that a new course is created upon updating the course ID, with the old one still being there. I believe this occurs because of the save_model logic that also updates the primary key.

    # When saving the course, update the id based on canvas id
    def save_model(self, request, obj, form, change):
        obj.id = canvas_id_to_incremented_id(obj.canvas_id)
        return super(CourseAdmin, self).save_model(request, obj, form, change)

I guess the edited object gets saved as a brand new object because of the new primary key, and it loses the connection to the course_view_option data. The backend does in fact throw a 500 error if you try to access the newly created course.

We could possibly modify save_model to clean this up, deleting the old object and copying over the course_view_option data. However, there's potential for data loss here, since if the cron was run, records attached to the deleted course will also be deleted. These could just be repopulated if the cron was run again, but the data_last_updated_at would also have to be emptied out.

As discussed in the meeting, it seems like it a more intuitive workflow for this would be to just delete the old course and create a new one with the new ID. We can disallow changing the course ID in the admin form to prevent the bugs described in the issue.

MyLA-2023.01.01 automation moved this from To do to Review/QA Jan 10, 2023
ssciolla added a commit that referenced this issue Jan 10, 2023
* Add field ordering; override change_view to make canvas_id readonly

* Add missing param
@jennlove-um
Copy link
Contributor

Testing passes in beta - the course ID is not editable.

@jennlove-um jennlove-um moved this from Review/QA to Done in MyLA-2023.01.01 Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ backend 🪳 bug Something isn't working
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants