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

Fix/improve workload overview #552

Merged
merged 7 commits into from
Oct 24, 2023
Merged

Conversation

EdoStorm96
Copy link
Contributor

I've made improvements to the workload overview of committee members, according to Desiree's wishes.

The queryset for get all open decisions had to be updated, because it was letting in the wrong things.
Also, to get all reviewers of a chamber, the code now finds all decisions in a chamber first and then looks at all associated reviewers. The old way also showed Michael for instance ...

The period for the second table can now also be customized, using two datepickers. This was a request of Desiree.

I've also incorporated a fix for issue #540, because it was a minor fix.

Copy link
Member

@tymees tymees left a comment

Choose a reason for hiding this comment

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

Logic looks good, the new queries do seem to make a lot more sense now.

The way you use the form could be better, luckily that's a fairly simple fix.

Lastly, it seems like you forgot to provide translations?

reviews/views.py Outdated
Comment on lines 89 to 111
class CommitteeMembersWorkloadView(
GroupRequiredMixin, CommitteeMixin, generic.FormView
):
template_name = "reviews/committee_members_workload.html"
group_required = [settings.GROUP_SECRETARY]

form_class = StartEndDateForm

def __init__(self, **kwargs):
super().__init__(**kwargs)
self.today = date.today()
self.one_year_ago = self.today - timedelta(days=365)

def get_initial(self):
initial = super().get_initial()

if "start_date" in self.request.GET and "end_date" in self.request.GET:
initial["start_date"] = self.request.GET.get("start_date")
initial["end_date"] = self.request.GET.get("end_date")
else:
initial["start_date"] = self.one_year_ago.strftime("%Y-%m-%d")
initial["end_date"] = self.today.strftime("%Y-%m-%d")

return initial
Copy link
Member

Choose a reason for hiding this comment

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

The way you're using the form here doesn't really make sense. It's basically a way to only display the form, which would probably be better done with just plain HTML at that point.

It works... But you're leaving a lot functionality on the table, most importantly validation.

I would suggest actually using the form-processing; This is done by making the form use 'post', and then overriding the 'post()' method of this View to retrieve the validated and cleaned data. For example:

    def __init__(self, **kwargs):
        super().__init__(**kwargs)
        self.today = date.today()
        self.start_date = self.today - timedelta(days=365)
        self.end_date = self.today

    def post(self, request, *args, **kwargs):
        form = self.get_form()
        if form.is_valid():
            self.start_date = form.cleaned_data['start_date']
            self.end_date = form.cleaned_data['end_date']
        else:
            return self.form_invalid(form)

        return self.get(request, *args, **kwargs)

    def get_initial(self):
        initial = super().get_initial()

        # You don't actually have to cast these to strings btw, DateField can handle date(time)s
        initial["start_date"] = self.start_date
        initial["end_date"] = self.end_date

        return initial

Then, use self.start_date and self.end_date in your querysets

@EdoStorm96
Copy link
Contributor Author

Thanks, Ty. That does look a lot smoother. I still struggle with comprehending forms ;p

One thing I ran into, is that on my system, if I don't add the string formatting of the initial dates, when I load the page in Dutch, the initial value of the dates defaults to (literally) "mm / dd / yyyy" instead of "10 / 23 / 2023" for example. It works fine when I load the page in English without the strftime but in Dutch it causes this bug. When I then submit the form, the dates will show correctly in the Dutch version as well. Do you think this is just a bug of my system? For now I've kept the string casting in the get_initial method.

@EdoStorm96 EdoStorm96 requested a review from tymees October 23, 2023 12:20
Copy link
Member

@tymees tymees left a comment

Choose a reason for hiding this comment

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

It's a bug in DSC

I can fix it there quite easily, but it will be a while before it can be used here. So, for now, your workaround is fine I guess?

I would like the comment above it to indicate it's a workaround for a bug. It's an approve for me, granted you do change it.

@EdoStorm96 EdoStorm96 merged commit b04fe9b into develop Oct 24, 2023
2 checks passed
@EdoStorm96 EdoStorm96 deleted the fix/improve_workload_overview branch October 24, 2023 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants