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

[bug][#11020] List admins changes not synchronized with admin_table #1

Merged
merged 1 commit into from
Mar 8, 2017
Merged

[bug][#11020] List admins changes not synchronized with admin_table #1

merged 1 commit into from
Mar 8, 2017

Conversation

salaun-urennes1
Copy link
Collaborator

Bug description: For some lists we have noticed a synchronization issue with the admin_table: an owner gets added to the list config file (through list family instanciation) but never appears in the admin_table. As a consequence this new list owner is not listed in the list panel on the web interface and does not have the expected privileges.

After a bit of code analysis and debugging I found out that revision 9948 is the cause of the problem: https://sourcesup.renater.fr/scm/viewvc.php/branches/sympa-6.1-branch/src/lib/List.pm?root=sympa&r1=9947&r2=9948

It turns out that List::sync_include_admin() subroutine mights never been run for a list if that list includes members from an external datasource but owner/editors get defined "statically" in the config list. To be more precise:

  • List::add_list_admin is called from List::sync_include_admin only
  • List::sync_include_admin is called
  • from List::new, unless a sync_include() has been ran recently
  • from task_manager::sync_include, but only if the list defined editor_include or owner_include parameters

This bug has on impact on any list that includes members from an external datasource but has owner/editor defined inline in the list config file. In our situation most lists based on list families are impacted. For these lists, list owners/editors never get updated in the admin_table.

Revision 9948 should be fixed to check $list->{'last_sync_admin_user'} instead of $list->{'last_sync'}; attached is a patch proposal.

Bug description: For some lists we have noticed a synchronization issue with the admin_table: an owner gets added to the list config file (through list family instanciation) but never appears in the admin_table. As a consequence this new list owner is not listed in the list panel on the web interface and does not have the expected privileges.

After a bit of code analysis and debugging I found out that revision 9948 is the cause of the problem: https://sourcesup.renater.fr/scm/viewvc.php/branches/sympa-6.1-branch/src/lib/List.pm?root=sympa&r1=9947&r2=9948

It turns out that List::sync_include_admin() subroutine mights never been run for a list if that list includes members from an external datasource but owner/editors get defined "statically" in the config list. To be more precise:
* List::add_list_admin is called from List::sync_include_admin only
* List::sync_include_admin is called
* from List::new, unless a sync_include() has been ran recently
* from task_manager::sync_include, but only if the list defined editor_include or owner_include parameters

This bug has on impact on any list that includes members from an external datasource but has owner/editor defined inline in the list config file. In our situation most lists based on list families are impacted. For these lists, list owners/editors never get updated in the admin_table.

Revision 9948 should be fixed to check $list->{'last_sync_admin_user'} instead of $list->{'last_sync'}; attached is a patch proposal.
@ikedas
Copy link
Member

ikedas commented Mar 8, 2017

I have no objection. PR by Salaun would be accepted.

@eiro
Copy link

eiro commented Mar 8, 2017

same here. thanks olivier

@eiro eiro merged commit 6ab6154 into sympa-community:sympa-6.2 Mar 8, 2017
@salaun-urennes1
Copy link
Collaborator Author

Thanks for the merge.
We now have some experience with that patch on production environment and I will probably have some additional changes to suggest:

  • while re-instantiating families List::sync_include_admin() is not triggered. It would make sense to add a call to List::sync_include_admin() towards the end of Family::modify_list(), just like List::sync_include(), except that List::sync_include_admin() should be run even if no owner_include or editor_include are defined for that list. List::sync_include_admin() will trigger synchronization of "static" list owners/editors with the admin_table in DB;
  • in List::new() the condition to execute List::sync_include_admin() ($list->{'last_sync_admin_user'} < time - $pertinent_ttl) is well adapted for owner_include or editor_include. But this condition does not suite well if owner/editor parameters were updated in the list config file; we expect that kind of changes to be pushed to the admin_table more quickly. Currently List::sync_include_admin() does two things: 1) reloads owner_include/editor_include data sources and 2) updates the admin_table for "static" and included list admins. Maybe we need a different subroutine to trigger (2) that might be called whenever the list config file changes...

@eiro
Copy link

eiro commented Mar 8, 2017

those points must be debated on the sympa-developpers list ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants