Bugfix delete button doesn't work for project#160
Conversation
maciejSamerdak
requested review from
Karrp,
MartynaAnnaGottschling,
Szymiks,
kbeker and
rwrzesien
managers/views.py
Outdated
|
|
||
| def get_success_url(self) -> str: | ||
| return reverse("custom-projects-list") | ||
| def delete_project(request, pk): |
There was a problem hiding this comment.
How this change is fixing the bug?
There was a problem hiding this comment.
Basically, DeleteView requires template to work. It serves as a confirmation page (we implemented this functionality as popup in previous view). With no template provided, the app crashes. With any template provided, a GET request is returned after closing popup in previous view and thus the template is rendered with no deletion being performed (because delete view is still waiting for POST to do that). That's definitely not how it should work at all.
At the time I was supposed to apply the fix ASAP, so it was a lot quicker for me to rewrite it to function rather than tangle with Django's mysterious inner mechanics in it's high-level View classes.
It is doable of course, I'd guess we have to overwrite get method so it calls for deletion and then redirects to desired address.
However, from our personal experience (since I developed entirety of eMunchkin based on generic views on my side and I strongly urged others to do the same) it was a lot more time consuming to figure out how the generic views work to know where exactly we must apply changes in functionality that suit our needs (which granted, was a learning experience at the time), than just writing it from scratch, or at the very least from lower-level abstraction.
Since the DeleteView is designed to suit much more sophisticated function than we require, which is to delete given object upon call and redirect (it literally works like a basic function) I'd strongly encourage to leave it as a function-based view, due to it's simplicity. Remodelling inner workings of complex Django View to suit such simplistic function-like purpose, is a major overkill in my opinion.
I am well aware of benefits regarding the principle of keeping all views in same technology, but I'd argue this is a valid exception. I wouldn't use complex mechanics for simplified actions. I think it's an unnecessary waste of resources, both in time and code.
This is also the exact reason we had to write our own views derived from ApiView rather than using generic views when we were still working on REST framework.
There was a problem hiding this comment.
Basically, DeleteView requires template to work.
No, it depends if we want to use both GET and POST. Here we only use POST.
With any template provided, a GET request is returned after closing popup in previous view and thus the template is rendered with no deletion being performed (because delete view is still waiting for POST to do that).
I honestly don't understand this part. Do you mean that the problem is that popup is making GET call or that the view is returning redirect to it? Because both can be changed either in popup itself, or in view - it can return redirect, template, or ajax response.
At the time I was supposed to apply the fix ASAP, so it was a lot quicker for me to rewrite it to function rather than tangle with Django's mysterious inner mechanics in it's high-level View classes.
But it has nothing to do with the bug itself. The bug is about missing jquery-ui script that doesn't allow popup to open. The view is working OK.
it was a lot more time consuming to figure out how the generic views work to know where exactly we must apply changes in functionality that suit our needs
Well, that the part of this project to get you guys familiar with how it works. And you don't actually have to do this, because it is already implemented, so you can just optionally get familiar with how it works.
Remodelling inner workings of complex Django View to suit such simplistic function-like purpose, is a major overkill in my opinion.
I wouldn't use complex mechanics for simplified actions.
They are simple generic views that covers most of the possible use cases with overriding 1-2 methods. Getting familiar with how they work and getting used to use them allows you to cover this cases with few lines of code.
I think it's an unnecessary waste of resources, both in time and code.
Again, this is already done.
There was a problem hiding this comment.
I honestly don't understand this part. Do you mean that the problem is that popup is making GET call or that the view is returning redirect to it? Because both can be changed either in popup itself, or in view - it can return redirect, template, or ajax response.
Currently we are getting there by link in tag. If a request method can be specified I wasn't aware of it (though it does seem obvious). By default, a GET was sent and the app crashed.
But it has nothing to do with the bug itself. The bug is about missing jquery-ui script that doesn't allow popup to open. The view is working OK.
After I made the button work again, the app crashed upon call for deleting. Since those two are tightly related, I figured I could quickly fix this newfound bug in same Pull Request.
But since the only change I applied was moving the scripts to different place in code, I'm certain the DeleteView still wouldn't work, because the way it's called from project_form didn't change.
Well, that the part of this project to get you guys familiar with how it works. And you don't actually have to do this, because it is already implemented, so you can just optionally get familiar with how it works.
If we treat it as a learning experience, sure. I'll be happy to dive into these mechanics and figure them out. I already had a good taste of it during eMunchkin development.
But recently, there is a lot of pressure regarding development, since we're struggling to deliver core functionality by the end of the month. This is why I was eager to pick faster solution that I'm familiar with, no to mention the time constraint at that particular time.
There was a problem hiding this comment.
Speaking of learning, I have a question: If you were to choose between generic view and function-based view in this case I take it you'd go for generic view. May I ask why is that specifically? I'd think that if the view's functionality is simple enough for a single, easily readable function, it'd be a better choice. I'd consider using complex class in such case to be at the very least a waste of memory. After all, when an instance is created, all of those unused features from parent class (like entirety of GET request handling) must be loaded, if I'm thinking correctly, making this view more resource consuming than it would be as a function. I am aware those differences are insignificant with today's tech, but to me it's a good design principle. Especially if function-based implementation was faster to develop. Thus I'm wondering, what are the benefits of deriving such view from generic class?
There was a problem hiding this comment.
@maciejSamerdak @kbeker @Szymiks read this too please.
This is why I was eager to pick faster solution that I'm familiar with, no to mention the time constraint at that particular time.
Sure, that's understandable. If we have situation like this and you are waiting for me you may always merge current state when its working but please add an issue in github so we can remember to come back to this later.
I am aware those differences are insignificant with today's tech, but to me it's a good design principle. Especially if function-based implementation was faster to develop. Thus I'm wondering, what are the benefits of deriving such view from generic class?
About benefits:
- Once you know how to use particular class it is very simple to create a new subclass of it and replace only the parts that you need. When you write function based view you have to provide full implementation each time. This is especially good for those simple, repeatable views that Django provides as generics. So I can't agree that its faster once you learn it, I think its opposite.
- When you are subclassing a generic view from popular framework like Django you can be more sure that it is implemented without bugs and well tested. It is always easier to make a bug when you create your own implementation (I don't mean "you", I mean anyone).
- You can use multiple inheritance. You can create your own generic classes which inherits from Django and does your custom logic and reuse them. You can use mixin classes. Basically you can use all the power of object oriented programming (which I hope we will have chance to use once we move all stuff to Django views).
- I think they are more readable. In example you can see you have a subclass of
TemplateViewwith custom template as an attribute and you already know everything about what this view does.
After all, when an instance is created, all of those unused features from parent class
Well this was actually a little untypical case that we wanted to use only POST part of this view, regularly you would use it fully. And if you need something in the middle you can always add it by just overriding some already existing method, without a need to modify whole function structure.
Those are just first things that comes to my mind. Off course this subject is probably better described in Django-related literature too. You can search for "class based views vs function based views django" and see different opinions and arguments.
I'd consider using complex class in such case to be at the very least a waste of memory.
Its good that you think about things like this but I would not worry too much about it unless we get to that point. I mean, as you have said, with today's tech its rather unnoticeable in web project with this target usage. I think worse bottlenecks could be number/complexity of SQL queries, size of http response or static files.
| {% load crispy_forms_tags %} | ||
|
|
||
| {% block content %} | ||
|
|
There was a problem hiding this comment.
Extra empty lines are not needed.
There was a problem hiding this comment.
Yeah, it's a styling error on my part. I just didn't have any time left to fix that.
| <b>{{object.name}}</b> {% trans 'project' %}? | ||
| </div> | ||
|
|
||
| <link rel="stylesheet" href="//code.jquery.com/ui/1.12.1/themes/smoothness/jquery-ui.css"> |
There was a problem hiding this comment.
This should be placed in {% block extra_head %} above {% block content %}
There was a problem hiding this comment.
After fixing the issues in base.html does this still apply or should it all be moved to the {% block extra_script %}?
There was a problem hiding this comment.
css'es should be in {% block extra_head %} which should be located at the end of <head> in base.html.
| </div> | ||
|
|
||
| <link rel="stylesheet" href="//code.jquery.com/ui/1.12.1/themes/smoothness/jquery-ui.css"> | ||
| <script src="//code.jquery.com/ui/1.12.1/jquery-ui.js"></script> |
There was a problem hiding this comment.
This is not correct solution for this.
The main problem is that {% block extra_script %}{% endblock %} was removed from the base.html template, so it must be added there first, at the bottom, above closing </body> tag. It is placed there because all scripts should be loaded after the rest of the html page is rendered. If they are placed in the head, selectors might apply to things that are not rendered yet. Off course they might be also applied just after the HTML code to which they are related, but it is good practice to have them in single place, not scattered around the page.
Second thing is that <script src="//code.jquery.com/ui/1.12.1/jquery-ui.js"></script> is missing so please add it to the {% block extra_script %}{% endblock %} in project_form.html template.
Please also move existing scripts from <head> above the {% block extra_script %}{% endblock %} in base.html.
Missing {% block extra_script %}{% endblock %} is also causing datepickers to not work, and I am sure they were working so this block must have been removed accidentally
There was a problem hiding this comment.
When we had {% block extra_script %} at the bottom of the base, the javascript dependent project joining feature crashed on page load making it impossible to use it. Report deletion feature was also disabled due to same error. This was because the scripts on those pages (and I mean the custom .js files themselves) were loaded inside the tag in these views and thus they where crashing, because all of dependencies in base were not yet loaded.
I am familiar with principle of loading the scripts only after the rest of the template is loaded to boost the rendering time. I apologise for resulted mess in 'base.html' due to my interference, but project join feature is essential for employee and this bug was detected on the day of release. We absolutely couldn't have released the app in such state and thus I was required to find another ASAP fix.
If I understand correctly, every single javascript reference in all templates must be moved to corresponding {% block extra_script %} below the rest of the code? I will apply the changes shortly and I'll check if all features work as expected.
There was a problem hiding this comment.
apologise for resulted mess in 'base.html' due to my interference
Sure don't worry, this project is too about learning right? :)
If I understand correctly, every single javascript reference in all templates must be moved to corresponding {% block extra_script %} below the rest of the code? I will apply the changes shortly and I'll check if all features work as expected.
Yes, if you want you can refactor existing templates in separate PR. All scripts should be moved to this block, while all css'es into extra_head.
rwrzesien
left a comment
rwrzesien
left a comment
There was a problem hiding this comment.
The approach is not correct, please read my comments and apply.
|
Since these changes seem to cause more harm than good, I guess I should drop these commits and start over with applying fixes to |
The way the script was implemented it would sent a GET request, resulting in crash due to TemplateDoesNotExist error. Instead of switching `windows.location.href` value, the script now submits a form with POST method, which is invisible on display.
maciejSamerdak
force-pushed
the
bugfix-delete-button-doesnt-work-for-project
branch
from
1a55d18 to
28340fe
Compare
4 participants
Fixes acknowledged issues regarding project deletion feature.
For specific details, please refer to commit messages.
Resolves #156
Resolves #155