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 sorting arrows for allocation search #344

Merged
merged 3 commits into from
Jan 11, 2022

Conversation

mdzik
Copy link
Contributor

@mdzik mdzik commented Dec 22, 2021

Was rendered as:

allocation/?order_by=end_date&direction=des&username=piospi&resource_type=Cluster&resource_name=1&show_all_allocations=True&

but should be this:

allocation/?order_by=end_date&direction=des&username=piospi&resource_type=2&resource_name=1&show_all_allocations=True&

@mdzik mdzik changed the title Fix sorting arrows for allocation seerach Fix sorting arrows for allocation searach Dec 22, 2021
@mdzik mdzik changed the title Fix sorting arrows for allocation searach Fix sorting arrows for allocation search Dec 22, 2021
@aebruno
Copy link
Member

aebruno commented Dec 23, 2021

I can reproduce this bug however the PR is failing for me with:

NameError at /allocation/

name 'Model' is not defined

Perhaps we can just check for the existence of the 'pk' attribute here? For example, changing the logic to this works for me:

if isinstance(value, QuerySet):
    for ele in value:
        filter_parameters += '{}={}&'.format(key, ele.pk)
elif hasattr(value, 'pk'):
    filter_parameters += '{}={}&'.format(key, value.pk)
else:
    filter_parameters += '{}={}&'.format(key, value)

@aebruno aebruno self-requested a review December 23, 2021 14:50
Copy link
Member

@aebruno aebruno left a comment

Choose a reason for hiding this comment

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

Fix model import error and/or adjust logic to check for existing of pk attribute.

@mdzik
Copy link
Contributor Author

mdzik commented Jan 4, 2022

I didn't notice that we have
import ... Model
in our branch, sorry.

But: i still have 'a feeling' that something is not exactly right in this search (like search by date) - will do some test to confirm

@mdzik
Copy link
Contributor Author

mdzik commented Jan 10, 2022

Ok - I've found it. The problem lays in how Calendar widget handles date format. If you have different time locale in use, than date-time format needs to be compatible between Django and JS widget, and JS requires slightly different format than Django. When localization is turned on, Datepicker widget needs to have something like:

{% get_datepicker_format as df %}
$('#id_grant_start').datepicker({ dateFormat: "{{ df }}" });

where

@register.simple_tag
def get_datepicker_format():
    return get_format('DATE_INPUT_FORMATS')[0].replace('%','').replace('m','mm').replace('Y','yy').replace('d','dd')

@aebruno
Copy link
Member

aebruno commented Jan 11, 2022

Ok - I've found it. The problem lays in how Calendar widget handles date format. If you have different time locale in use, than date-time format needs to be compatible between Django and JS widget, and JS requires slightly different format than Django.

Awesome, thanks for the additional info! Did you want to add those changes to this PR? if so, I'll hold off on merging. Thanks!

@mdzik
Copy link
Contributor Author

mdzik commented Jan 11, 2022

I think that is another issue and it is not present if you don't "change language". I just left it here 'for future generations'

@aebruno aebruno merged commit e0ead55 into ubccr:master Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants