bug #21319 - errors in separate_translation mechanism as of 070618-1

[boud]

hi Dmitry,

On Mon, 18 Feb 2008, Dmitry Borodaenko wrote:

> On Feb 17, 2008 2:32 AM, boud <address@hidden> wrote:
> > crosslink to patch 5882 =
> > bug-fix: separate_translation patch 0.4.2 (vs samizdat-0.6.0.070618-1)
> > https://savannah.nongnu.org/bugs/?21319
> > https://savannah.nongnu.org/patch/?5882
>
> The only reason I can imagine to have both a bug and a patch open is
> if the patch fixes several different existing bugs with the same code
> (as in, fixes can't be isolated). Why open a bug that merely points to
> an existing patch?

My worry was that since there was nothing listed in the list of bugs,
people might miss it, even though it's a set of (related) bugs.

> I have reviewed the patch #5882 (separate translations 0.4.2), it's
> not suitable for merging in: code duplication is never good, and this
> patch introduces identical changes to a whole lot of different places.

i agree that code duplication is against the whole spirit of ruby, and
i agree that my patch would require a lot of improvement and
compactification before merging in. On the other hand, the problem
is important and IMHO is a quite high priority for being fixed.


> The patch also appears to fix several unrelated issues in one go.

This i don't see, except for the comment on #20303, but that's just in
the latest comment.  There may be one or two lines in the patches
which are independent from the translation issue, but IMHO most of
them are part of the same issue: the user clicks (or cannot click due
to missing button(s)) and expects something dealing with the message
that s/he saw, but instead gets an equivalent message (source or
rendered) but in another language (if the initial translation was
correctly done).

> The best way when submitting patches is to have a separate patch for
> each bug and feature. This way, even if some changes are
> controversial, other fixes will still have a chance to get in. Second
> best way (especially when you know yourself that your changes are
> controversial) is to clearly describe what is wrong and how you want
> it fixed, so that I can implement a fix myself.

OK - i think my error in terms of keeping things neat was that i tried
updating the "original" separate translation patch whereas i should have
started a new entry - probably as a bug with suggested fix?

> Just looking at the code of the fix doesn't help me understand the
> problem, and comments on the patch don't help much, either. Please
> provide use case scenarios that would demonstrate the bug and say what
> you see and what you expect to see instead.

The whole thread got quite long, but if you look through:
https://savannah.nongnu.org/patch/?5882

you'll find a link to:
http://lists.gnu.org/archive/html/samizdat-devel/2007-06/threads.html

and this email probably had the clearest description of the bug:
http://lists.gnu.org/archive/html/samizdat-devel/2007-06/msg00014.html

In any case, it turned out that there were more related problems than
in that original message, so i'll an update here to clarify the problems.

i'm not sure where i should put this. At least putting it here
on the mailing list can't hurt, since the issue has been raised and
IMHO is important. i guess that the neatest thing, if we don't solve
this soon, is to add this to bug 21319 and close up patch 5882.


=========================== BUG 21319 ===========================

PROBLEM: The problem is that although a user who only wishes to *read*
a message will correctly see the message in his/her preferred language
(the accept_language) without needing to know the true id of the
message ("true" meaning the id in the database), there are many situations
where in samizdat-0.6.0.070618-1 (and probably up to 080214-2), a user
doing something more complicated than just reading will get confused: s/he
does an operation on (clicks a button below) a message but then gets
a new html page which is inconsistent with what s/he expects (i.e. is
in a different language).

Listing all the different cases individually is not easy since it's a
large number - it seemed less work just to get the patch to work.

Most of the problems seem to occur when a parent (according to the
database) message is in a different language to the accept_language.
However, some attempts at correcting this can lead to errors for 
the case when the parent (in the database) message is in the same
language as the accept_language, so it's better to check all four
cases for each operation.

DIFFERENT USER CASES:

* anonymous user: history: several errors
* logged-in user: several operations lead to message in wrong language
* logged-in moderator: several operations lead to message in wrong language


FOUR SITUATIONS OF MESSAGES: The situation differs depending on
whether the message is a parent or child according to the database,
and whether it is a parent or child according to what the user sees in
his/her browser.

----------------------------------------------------------------------
* = in accept_language
entries = except for first two lines, these show the DATABASE point
          of view
----------------------------------------------------------------------
DATABASE point of view: parent*   child         parent     child*
USER point of view     parent    child         child      parent

BUTTON/operation to check:
file:

message_controller.rb:

Hide
confirm, Redirect

Unhide
confirm, Redirect

Reparent   (complex because parent-child relations change)
confirm, Redirect

Edit
confirm, Redirect

Takeover
confirm, Redirect

Replace
confirm, Redirect




resource_controller.rb:

Add another focus
  action_confirmed



message_helper.rb:

"history" link

"source" link

"hidden" non-link should be given if and only if the message from
the user's point is hidden

Edit button: 
* should be present if the user is the author of the
message shown in the browser (not the child/parent of the message shown)
* should be present if the message shown in the browser (not the
  child/parent of the message shown) is "open for editing to all members"

hide/unhide buttons:
* should depend on whether the message shown in the
  browser (not the child/parent of the message shown) is presently
  hidden/unhidden and clicking the link should lead to what looks like
  an operation on the message shown.

reparent/takeover/replace buttons:
*  should lead to what looks like an operation on the message shown,
   not on the child/parent of that message



resource_helper.rb:

vote:
* should operate on the parent message (database point of view)

----------------------------------------------------------------------



So it looks like there are about 14 different operations/buttons/links
and at least two cases (database=parent + user=child or vice-versa)
which for most of these are wrong in 070618-1, and in the other two
cases may be sometimes wrong (e.g. edit button incorrectly present or missing),
but it's a long time since i last checked them all.

This makes potentially about 56 different situations which need to 
be simultaneously (in terms of changes to code) done correctly.

There may be some relatively elegant efficient way to do this, but a
complete "live" check (e.g. 1 minute for each situation) is
necessarily time-consuming.


cheers
boud