PR#3478: add date custom field - pagure

pagure

#3478 add date custom field

Merged 5 years ago by pingou. Opened 5 years ago by karsten.

karsten/pagure issue-3410 into master

Add a Date type to the custom fields

Karsten Hopp • 5 years ago

063ec08

pagure/lib/query.py

file modified

+7 -5

		`@@ -4763,7 +4763,7 @@`

		`def set_custom_key_fields(session, project, fields, types, data, notify=None):`
		`""" Set or update the custom key fields of a project with the values`
		`- provided. "data" is currently only used for lists`
		`+ provided. "data" is currently only used for lists and dates`
		`"""`

		`current_keys = {}`
		`@@ -4771,12 +4771,14 @@`
		`current_keys[key.name] = key`

		`for idx, key in enumerate(fields):`
		`- if types[idx] != "list":`
		`- # Only Lists use data, strip it otherwise`
		`- data[idx] = None`
		`- else:`
		`+ if types[idx] == "list":`
		`if data[idx]:`
		`data[idx] = [item.strip() for item in data[idx].split(",")]`
		`+ elif types[idx] == "date":`
		`+ if data[idx]:`
		`+ data[idx] = data[idx].strip()`
		`+ else:`
		`+ data[idx] = None`

		`if notify and notify[idx] == "on":`
		`notify_flag = True`

pagure/templates/issue.html

file modified

		`@@ -50,6 +50,7 @@`
		`{% else %}`
		`<input`
		`{%- if field.key_type == 'boolean' %} type="checkbox" {% endif %}`
		`+ {%- if field.key_type == 'date'%} type="date" {% endif %}`
		`class="form-control" name="{{ field.name }}" id="{{ field.name }}"`
		`{%- if field.name in knowns_keys %}`
		`{% if field.key_type == 'boolean'%}`
		`@@ -544,6 +545,7 @@`
		`{% else %}`
		`<input`
		`{%- if field.key_type == 'boolean' %} type="checkbox" {% endif %}`
		`+ {%- if field.key_type == 'date'%} type="date" {% endif %}`
		`class="form-control" name="{{ field.name }}" id="{{ field.name }}"`
		`{%- if field.name in knowns_keys %}`
		`{% if field.key_type == 'boolean'%}`

pagure/templates/settings.html

file modified

+30 -7

		`@@ -713,19 +713,19 @@`
		`value="" class="form-control"/>`
		`</div>`
		`<div class="col-sm-2 pr-0">`
		`- <select name="custom_keys_type" class="form-control">`
		`+ <select name="custom_keys_type" class="form-control custom-keys">`
		`<option value="text">Text</option>`
		`<option value="boolean">Boolean</option>`
		`<option value="link">Link</option>`
		`<option value="list">List</option>`
		`+ <option value="date">Date</option>`
		`</select>`
		`</div>`
		`<div class="col-sm-6 pr-0">`
		`- <input title="Comma separated list items" type="text" name="custom_keys_data"`
		`- value="" class="form-control"/>`
		`+ <input title="Comma separated list items" type="text" name="custom_keys_data" value="" class="form-control custom-keys-list hidden" id="custom_keys_list"/>`
		`</div>`
		`<div class="col-sm-1 pr-0">`
		`- <input type="checkbox" name="custom_keys_notify" title="Trigger email notification when updated">`
		`+ <input type="checkbox" name="custom_keys_notify" title="Trigger email notification when updated" class="form-control"/>`
		`</div>`
		`<div class="col-sm-1">`
		`<a href="javascript:void(0)" class="btn btn-outline-danger remove-settings-field-row"><i class="fa fa-trash"></i></a>`
		`@@ -738,7 +738,7 @@`
		`value="{{ field.name }}" class="form-control"/>`
		`</div>`
		`<div class="col-sm-2 pr-0">`
		`- <select name="custom_keys_type" class="form-control">`
		`+ <select name="custom_keys_type" class="form-control custom-keys">`
		`<option value="text" {%`
		`if field.key_type == 'text' %} selected {%`
		`endif %}>Text</option>`
		`@@ -751,11 +751,19 @@`
		`<option value="list" {%`
		`if field.key_type == 'list' %} selected {%`
		`endif %}>List</option>`
		`+ <option value="date" {%`
		`+ if field.key_type == 'date' %} selected {%`
		`+ endif %}>Date</option>`
		`</select>`
		`</div>`
		`<div class="col-sm-6 pr-0">`
		`- <input title="Comma separated list items" type="text" name="custom_keys_data"`
		`- value={% if field.data is none %}""{% else %}"{{ field.data \| join(', ') }}"{% endif %} class="form-control"/>`
		`+ {% if field.key_type == 'list' %}`
		`+ <input title="Comma separated list items" type="text" name="custom_keys_data" id="custom_keys_list"`
		`+ value={% if field.data is none %}""{% else %}"{{ field.data \| join(', ') }}"{% endif %} class="form-control custom-keys-list"/>`
		`+ {% else %}`
		`+ <input title="Comma separated list items" type="text" name="custom_keys_data" id="custom_keys_list"`
		`+ value="{{ field.data or '' }}" class="form-control custom-keys-list hidden"/>`
		`+ {% endif %}`
		`</div>`
		`<div class="col-sm-1 pr-0">`
		`<input type="checkbox" name="custom_keys_notify-{{ loop.index }}" title="Trigger email notification when updated"`
		`@@ -1092,6 +1100,20 @@`
		`<script type="text/javascript" src="{{ url_for('static', filename='vendor/selectize/selectize.min.js') }}?version={{ g.version}}"></script>`

		`<script type="text/javascript">`
		`+ function updateform() {`
		`+ $('.custom-keys').change(function() {`
		`+ field_type = $(this).val();`
		`+ if(field_type == "list") {`
		`+ $(this).parent().parent().find('.custom-keys-list').removeClass("hidden");`
		`+ } else {`
		`+ $(this).parent().parent().find('.custom-keys-list').addClass("hidden");`
		`+ }`
		`+ });`
		`+ };`
		`+ updateform();`
		`+ </script>`
		`+`
		`+ <script type="text/javascript">`
		`function show_acls(acls) {`
		`var _txt = '<div title="ACLs details" id="show_meeting">'`
		`+ '<ul>';`
		`@@ -1153,6 +1175,7 @@`
		`$(this).parent().parent().remove();`
		`});`
		`$(target + ".settings-field-rows").append(row);`
		`+ updateform();`
		`console.log(row);`
		`});`

tests/test_pagure_flask_api_issue.py

file modified

+8 -4

		`@@ -3833,10 +3833,10 @@`
		`repo = pagure.lib.query.get_authorized_project(self.session, 'test')`
		`msg = pagure.lib.query.set_custom_key_fields(`
		`self.session, repo,`
		`- ['bugzilla', 'upstream', 'reviewstatus'],`
		`- ['link', 'boolean', 'list'],`
		`- ['unused data for non-list type', '', 'ack, nack , needs review'],`
		`- [None, None, None])`
		`+ ['bugzilla', 'upstream', 'reviewstatus', 'duedate'],`
		`+ ['link', 'boolean', 'list', 'date'],`
		`+ ['', '', 'ack, nack , needs review', '2018-10-10'],`
		`+ [None, None, None, None])`
		`self.session.commit()`
		`self.assertEqual(msg, 'List of custom fields updated')`

		`@@ -3851,6 +3851,10 @@`
		`self.assertEqual(`
		`sorted(key.data), ['ack', 'nack', 'needs review'])`

		`+ # Check that the duedate date field still has its date`
		`+ if key.name == "duedate":`
		`+ self.assertEqual(key.data, '2018-10-10')`
		`+`
		`# Check that not setting the value on a non-existing custom field`
		`# changes nothing`
		`output = self.app.post(`

tests/test_pagure_flask_ui_repo.py

file modified

+4 -4

		`@@ -1472,19 +1472,19 @@`
		`value="" class="form-control"/>`
		`</div>`
		`<div class="col-sm-2 pr-0">`
		`- <select name="custom_keys_type" class="form-control">`
		`+ <select name="custom_keys_type" class="form-control custom-keys">`
		`<option value="text">Text</option>`
		`<option value="boolean">Boolean</option>`
		`<option value="link">Link</option>`
		`<option value="list">List</option>`
		`+ <option value="date">Date</option>`
		`</select>`
		`</div>`
		`<div class="col-sm-6 pr-0">`
		`- <input title="Comma separated list items" type="text" name="custom_keys_data"`
		`- value="" class="form-control"/>`
		`+ <input title="Comma separated list items" type="text" name="custom_keys_data" value="" class="form-control custom-keys-list hidden" id="custom_keys_list"/>`
		`</div>`
		`<div class="col-sm-1 pr-0">`
		`- <input type="checkbox" name="custom_keys_notify" title="Trigger email notification when updated">`
		`+ <input type="checkbox" name="custom_keys_notify" title="Trigger email notification when updated" class="form-control"/>`
		`</div>''', output_text)`

		`def test_view_forks(self):`

karsten commented 5 years ago

Fixes: https://pagure.io/pagure/issue/3410
This adds a date entry to the pulldown menu in custom fields
clicking on it while modifying metadata of issues allows to pick a date.

Signed-off-by: Karsten Hopp karsten@redhat.com

1 new commit added

use date type

5 years ago

pingou commented on line 51 of pagure/templates/settings.html 5 years ago

Please use space rather than tabs :)

1 new commit added

use spaces instead of tabs

5 years ago

farhaan commented on line 66 of pagure/templates/settings.html 5 years ago

Looks like we can format it and put indentation here.

farhaan commented on line 70 of pagure/templates/settings.html 5 years ago

Meaningful variable name here please :smile:

farhaan commented on line 14 of pagure/lib/__init__.py 5 years ago

Is it absolutely necessary to use None?

karsten commented on line 18 of pagure/lib/__init__.py 5 years ago

None has been used here ^^^ before, that's the only reason I've used it, too

rebased onto eb9c22e267049ae754a8c651a944a072b64a9a7c

5 years ago

farhaan commented on line 47 of pagure/templates/settings.html 5 years ago

is the console log required?

karsten commented 5 years ago

only for the review to show that the class that hides the element got removed

rebased onto 780ba91b58ca5fc753fd3c924f54d5ea3b30dd57

5 years ago

rebased onto 7afdb19d199eaba30c7f1090dec5645a46bd8ec1

5 years ago

rebased onto b0441d6097fe910a31b79eb1eecf3b369d1fcd4b

5 years ago

pingou commented 5 years ago

There is something odd here, the value of the date field is filled in the settings page or in the ticket that gets this field?

From my local testing, it asks for a value (though doesn't enforce it) in the settings page, while I would expect that the value (thus the calendar widget) shows up when I try to set it in the issue page, no?

rebased onto 2a70e2d082cefb8f7627fbf4ba38ec6ce013608b

5 years ago

pingou commented 5 years ago

It looks like it was rebase but I don't think my previous comment was addressed, was it? (I may be missing it)

karsten commented 5 years ago

No, I've worked on the /pull-request/ready stuff as that had priority

rebased onto cecce7fe094ebed0891fbfbc66c26306fdf432d7

5 years ago

rebased onto cecce7fe094ebed0891fbfbc66c26306fdf432d7

5 years ago

rebased onto 3b79a954adb389516b0f30bd05902c875943d3d3

5 years ago

rebased onto ef30f2487668e8d17f3e12b73469885836749fc4

5 years ago

karsten commented 5 years ago

Test passed, please review.
I've removed the date input field from the settings page and moved it to the issues metadata tab.

rebased onto 4eb08f8ff3e6d3c514f5e6f5abe83420bcd3bc0b

5 years ago

pingou commented on line 56 of pagure/templates/settings.html 5 years ago

I'm confused about these two lines

rebased onto 570d9fb9a552ad84f5ca5ae645bb6de2ea913d7d

5 years ago

karsten commented 5 years ago

I'm confused about these two lines

They make sure that at least an empty string gets returned, otherwise you'll get failures in pagure/lib/__init__.py around line 4773 when the number of data entries doesn't match the number of types entries.

Edited 5 years ago by pingou

pingou commented on line 56 of pagure/templates/settings.html 5 years ago

We could drop the title, class and if the key_type is not a list, is there a reason to do the join(', ') ?

pingou commented on line 59 of pagure/templates/settings.html 5 years ago

Shouldn't we use elif instead of if/else/if/else ?

pingou commented on line 64 of pagure/templates/settings.html 5 years ago

I'd drop the title if this is hidden, same for the css class.
The value could also likely be simplified to {% field.data or "" %}

karsten commented on line 64 of pagure/templates/settings.html 5 years ago

I've changed that to "{{ field.data or '' }}". The css classes needs to stay, otherwise the input filed has the wrong size when you switch from one key_type to 'list'

karsten commented on line 59 of pagure/templates/settings.html 5 years ago

I've dropped the 'Date' stuff completely, not required here

karsten commented on line 56 of pagure/templates/settings.html 5 years ago

title and class need to stay, otherwise you won't get the input field mouseover hint and the wrong field size when you create a text field, press update and then decide to change that to 'list'

rebased onto 136079dd371ec6a56045466ef19f388773cb80d3

5 years ago

1 new commit added

modify test for the fixed notify button class

5 years ago

rebased onto 24a59a91e9107a8151ee1054220b378d130ead3f

5 years ago

pingou commented 5 years ago

Looks all good and jenkins is happy to, the only thing I see is: The format when adding the date is MM/DD/YYYY while it's shown as yyyy-dd-mm ? Could we change the input field? (Even if only to YYYY/MM/DD)

If not, I'll merge as is :)

karsten commented 5 years ago

Unfortunately not that easily. Most browsers show a calender picker widget where this doesn't matter. Your browser doesn't seem to support that and what text is shown depends on the browser and language settings.

pingou commented 5 years ago

Your browser doesn't seem to support that and what text is shown depends on the browser and language settings.

It does show the calendar widget but the place holder is US format unlike what we display.

If it's related to the browser and language settings then I'm fine. Let's get this in :)

pingou commented on line 30 of pagure/lib/__init__.py 5 years ago

Actually, is this change still needed?

rebased onto 136079dd371ec6a56045466ef19f388773cb80d3

5 years ago

rebased onto 28e12455035cfd781248b353195538fbb9b8fd4b

5 years ago

rebased onto 063ec08

5 years ago

pingou commented 5 years ago

I've adjusted this PR a little bit (tests were failing) and I squashed all commits into one.
I'm running the tests locally and if they pass, I'll merge :)

Thanks @karsten for your work on this! :)