maxrossello / redmine_extended_watchers

Grant additional issue and project view permissions to watcher users
GNU General Public License v3.0
44 stars 20 forks source link

Возможное решение Issue Plugin conflicts #27 #33

Closed openncomp closed 2 years ago

openncomp commented 2 years ago

Здравствуйте! Во-первых, от всего сердца благодарю вас за этот замечательный плагин и за то, что вы его поддерживаете!


Я не программист в общем и возможно напишу много текста, но мне пришлось столкнуться с Редмайном и всеми его "прелестями".

Я использую некоторые бесплатные плагины от RM+. В большинстве своем они используют плагин "a_common_libs" для своей работы, который давно не обновлялся у них на сайте, а их официального репозитория я нигде не нашел.

Но зато нашел ваш плагин и столкнулся с несовместимостью "redmine_extended_watchers" и их связкой плагинов "a_common_libs"+"unread_issues". Нашел подобный вопрос у вас - #27. Попробовал способы, которые там описаны, но они не помогли решить проблему несовместимости. Тем более, в том вопросе писали, что их плагины не совместимы с Redmine 4.x.

Со временем я нашел совместимые с Redmine 4.x версии плагинов: a_common_libs unread_issues

Но после перезапуска Redmine, я получил ошибку Internal error при доступе к веб интерфейсу. В логах были записи:

App 13940 output: plugins/a_common_libs/lib/acl/patches/models/user_patch.rb:158:in `allowed_to_with_acl?'
App 13940 output: plugins/redmine_extended_watchers/lib/extended_watchers_user_patch.rb:23:in `allowed_to?'

и

App 14572 output: plugins/a_common_libs/lib/acl/patches/models/issue_patch.rb:171:in `visible_condition_with_acl'
App 14572 output: plugins/redmine_extended_watchers/lib/extended_watchers_issue_patch.rb:50:in `visible_condition'

Следуя простой логике, я посчитал, что разные переменные не должны иметь одно и то же описание, и заменил в вашем плагине:

plugins/redmine_extended_watchers/lib/extended_watchers_user_patch.rb:
allowed_to --> allowed_to_with_acl

и

plugins/redmine_extended_watchers/lib/extended_watchers_issue_patch.rb:
visible_condition --> visible_condition_with_acl

После перезапуска, веб интерфейс загрузился без ошибок, в логах тоже чисто. На первый взгляд оба плагина работают как полагается.

Я не знаю, насколько это правильный способ и не несет ли он в себе какие-то скрытые угрозы. Я хотел бы, чтобы вы посмотрели на это решение и если оно не верное, подсказали как сделать правильно. Вы также могли бы включить решение этой проблемы в свои дальнейшие выпуски.

P.S. Если вам будет нужен перевод моей заявки, я могу объясниться проще через переводчик на ваш родной язык, просто напишите об этом.


Вот информация о моей сборке Redmine:

Environment:
  Redmine version                4.2.3.stable
  Ruby version                   2.6.6-p146 (2020-03-31) [x86_64-linux]
  Rails version                  5.2.6
  Environment                    production
  Database adapter               Mysql2
  Mailer queue                   ActiveJob::QueueAdapters::AsyncAdapter
  Mailer delivery                smtp
SCM:
  Subversion                     1.10.4
  Git                            2.20.1
  Filesystem                     
Redmine plugins:
  a_common_libs                  2.5.7
  additionals                    3.0.3
  nxs_chat                       2.0
  redmine_agile                  1.6.2
  redmine_base_deface            1.5.3
  redmine_checklists             3.1.19
  redmine_cms                    1.2.2
  redmine_contacts               4.3.4
  redmine_default_assignee       1.1.0
  redmine_drawio                 1.1.4
  redmine_extended_watchers      4.2.1
  redmine_hide_estimated_hours   1.0.14
  redmine_impersonate            2.0.0
  redmine_incoming_emails        0.0.1
  redmine_issue_dynamic_edit     0.7.2
  redmine_mentions               0.0.1
  redmine_more_previews          2.0.7
  redmine_priorities_duedate_js  0.1.0
  redmine_questions              1.0.2
  redmine_shortcuts              0.5.1
  redmine_silencer               0.4.3
  redmine_theme_changer          0.4.0
  sidebar_hide                   0.0.8
  that_email_log                 0.0.4
  that_issue_reply_button        0.0.1
  that_resent_notification       0.0.1
  unread_issues                  2.2.4
  vault                          0.4.3

Еще раз спасибо за ваш труд.

maxrossello commented 2 years ago

Hi, yes, if you don't mind I think that english is the standard "universal" language on github (I am natively italian), and using that eases the understanding for anyone bumping on a similar problem.

Actually, plugin compatibility poses a problem to Redmine, especially because the automated tests start to fail when some behavior is changed by some plugin, so everything is left to plugin's tests executed in isolation (when available). This is why I created https://github.com/maxrossello/redmine_testsuites, which adapts the automated tests supporting a set of optional plugins which are, therefore, certified to be working together.

I can't of course provide support for compatibility with any other arbitrary plugin. However, I did a little investigation about your case. The problem here is that the a_common_libs plugin modifies some of the same services, which you reported, as this plugin, but although still apparently working in Rails 5 in isolation, it carries out the task through a deprecated API. You may read a bit more about the old alias_method_chain approach and the newer replacement (prepend) here.

Alias_methodchain creates two aliased methods (one "..._with_..." and one "..._without\...") which respectively represent the new method executed on a call of the original reimplemented method, and the second represents the new way through which it is possible to call the older unmodified method. Conversely, the prepend method puts a module in front of the original class, so the method name does not change, and the original one still exists as is and can be called through super. This is a much better object oriented approach.

Both plugins call the original method in order to modify some bit of behavior yet keep the rest as it may change on main Redmine. The problem is that the two methods are not compatible because they introduce different naming to the same methods. So it happens that "method_with_change" when it calls "method_without_change", actually ends up to call "method" as implemented by this plugin. But since also this method calls super, and the method on the base class is aliased to "method_with_change", then it happens that the two call each other in an infinite loop until the stack is too large and thus the system crashes.

The workaround that you applied works because you create an alignment about the supposed method name; but this comes at the cost that, if a_common_lib plugin is not installed, then my plugin would not work, because it would modify a method that does exist only because of the alias. Furthermore, in general every single overridden method would require a special management in case some other plugin aliases it using a deprecated approach, so it is of course not my intention to provide this compatibility layer.

Finally, besides the disappearing of explicit errors, I wonder whether that plugin would really work as expected, since _alias_methodchain was deprecated in Rails 5.0 and removed in Rails 5.1.

So, in the end, unfortunately the incompatibility between the two plugins is clear and there's not much to do in general terms. It's also surprising that the other anyway works on plain RM 4.2. I suggest you may ask the other plugin's developer to refresh his implementation and make it last longer through the future: see https://littlelines.com/blog/2018/01/31/replace-alias-method-chain .

Good luck!