Roundup Tracker - Issues

Issue 2550964

classification
History can (temporarily) show incorrect value when a change is rejected
Type: behavior Severity: normal
Components: Web interface Versions: devel
process
Status: fixed fixed
:
: rouilj : cmeerw, jerrykan, rouilj
Priority: normal : Blocker, NeedsTesting, patch

Created on 2018-07-07 22:33 by rouilj, last changed 2021-05-26 16:15 by rouilj.

Files
File name Uploaded Description Edit Remove
BLOCK_SUBJECT.py rouilj, 2018-07-07 22:33
Messages
msg6101 Author: [hidden] (rouilj) Date: 2018-07-07 22:33
Set up a classic tracker with the attached detector.

1 Create an issue and save it.
2 Now change the Title and commit the change.

Look at the history and you will see the change recorded. The right
hand side of the change will be the current title (state A).

3 Now change the title to include the word rejectme.

You should see an error:

  Edit Error: subject rejected because keyword rejectme used

if you look at the history at the bottom of the issue, you will see
the new title with the word rejectme where the original title was (state B).

E.G.

at A you see:

2018-07-07 18:06:04	admin	set	title: Do you like foo foo -> Do you like
foo bar
2018-07-07 18:00:36	admin	set	title: Do you like foo -> Do you like foo foo

at B you see:

2018-07-07 18:06:04	admin	set	title: Do you like foo foo -> Do you like
foo bar rejectme
2018-07-07 18:00:36	admin	set	title: Do you like foo -> Do you like foo foo

The rejected title is *not* committed to the database.
This can be seen with roundup-admin history issueX or display issueX.
So this is not the same as issue2550955.

I think the code that preserves changes in the web page is causing the
history mechanism to display the web form data and not the data in the
database.

So the "current" history element that is displayed on the right hand
side is the web form value.

It should be possible to insert TAL before the history generation
section of the template to retrieve/display the database value.
E.G. adding:

<span tal:omit-tag="python:True"
 tal:content="python:utils.set_form_wins(request.client,False)">
<span>

before the call to context.history in the template seems to fix it
in my test. So need to add to every template.

Fixing the rejected change fixes this issue. As a result I consider this
a cosmetic issue, so not marking it as a blocker.
msg6102 Author: [hidden] (rouilj) Date: 2018-07-07 22:39
Realised that tal code is calling a function that I added to my
extensions/template.py file.

Add:

def set_form_wins(client,value):
    '''set client's form_wins attribute to the boolean value
       passed in.
    '''
    client.form_wins=value

to the file and add:

    instance.registerUtil('set_form_wins', set_form_wins)

to the init() function in the file.
msg6865 Author: [hidden] (jerrykan) Date: 2020-01-20 02:20
The msg6860 and msg6861 comments in issue2550612 may be relevant to this issue.
msg7247 Author: [hidden] (rouilj) Date: 2021-05-26 01:35
Hmm this seems to work as well:

<tal:block tal:define="wins 
python:utils.set_form_wins(request.client,False)"
	   tal:condition="context/id"
	   tal:replace="structure context/history" />


since tal:define runs before any other tal attributes. This
eliminates an additional span.

However with the span I could throw in a comment:

<span tal:omit-tag="python:True"
 tal:content="python:utils.set_form_wins(request.client,False)">
 Make sure that the history info is determined from the database and
 that form values don't bleed over.
<span>

that is not possible using define. Any ideas on how to document this
use of define?

-- rouilj
msg7248 Author: [hidden] (rouilj) Date: 2021-05-26 02:04
Christof, I need to figure out how to call a function to make
the history command only report info in the database.
Currently the data from a rejected form leaks into the history.

What happens if I put

  {{ utils.set_form_wins(context,False) }}

before calls to {{ context.history() }}?

I don't think set_form_wins returns anything but I am not sure. I
would like a way to run something in jinja where the result is
discarded and not emitted to the page. It doesn't appear that jinja
statement expressions are enabled.

If I wanted to enable statement expressions so I can use:

   {{ do utils.set_form_wins(context,False) }}

would it be as simple as adding a new element to:

  extensions=['jinja2.ext.i18n'],

in cgi/engine_jinja.py??

I am asking as you did the last changes in that area.

Thanks and have a great week.
msg7249 Author: [hidden] (rouilj) Date: 2021-05-26 16:15
I've been handling this wrong. Fixed in changeset:   6422:91ae685405ba

I fixed templating.py:history(). It should always use only the
database values, so set self._client.form_wins to False for the
duration of the method. Set to original value on return.

This replaces a ton of changes to templates. That was the wrong way
to "fix" this.

Also for the record modifying:

 templating/engine_jinja2.py Jinja2Loader::__init__

 to use:

   extensions=['jinja2.ext.i18n', 'jinja2.ext.do']

makes:

  {% do utils.set_form_wins(request.client, False) %}

work as expected producing no output.

Christof, should we add some other jinja2 extensions or expose it
via the config.ini mechanism? This extension seems to mimic the
tal:omit-tag attribute.

Again this change falls into the black hole in the test framework.
So marked NeedsTesting.
History
Date User Action Args
2021-05-26 16:15:19rouiljsetkeywords: + NeedsTesting
assignee: rouilj
status: new -> fixed
messages: + msg7249
resolution: fixed
2021-05-26 02:04:10rouiljsetnosy: + cmeerw
messages: + msg7248
2021-05-26 01:35:23rouiljsetmessages: + msg7247
2021-05-06 19:19:45rouiljsetkeywords: + Blocker
2020-01-20 02:20:57jerrykansetnosy: + jerrykan
messages: + msg6865
2018-07-07 22:39:44rouiljsetmessages: + msg6102
2018-07-07 22:33:25rouiljcreate