SYNCOPE-1506: Merge Accounts

[mmoayyed]

No description provided.

[mmoayyed]

This should be ready to merge, unless there are further changes required. I can also take a pass at writing wicket tests for the new screen to merge accounts, if that is appropriate.

[ilgrosso]

Thanks @mmoayyed , code looks ok but I would like to take it out for few smoke tests.

I can also take a pass at writing wicket tests for the new screen to merge accounts, if that is appropriate.

This would be really appreciated, thanks.

[mmoayyed]

Been working on tests; still WIP; This should also be done.

[ilgrosso]

@mmoayyed I have been running this PR and have the following notes:

1. the new Merge Accounts modal window's footer is too high, there might be something weird with modal setup
2. modal window's title should mention the receiving user for merge, e.g. the one that was initially selected
3. all users but the receiving user should be good candidates to be selected for merge, regardless they have linked accounts or not
4. some kind of preview feature before commit is desirable: consider that such operation is going to remove a user, hence admins should be adequately warned
5. what about using https://fontawesome.com/v4.7.0/icon/compress as menu icon?

Thanks!

[mmoayyed]

Thanks much for the review. Quick progress report:

the new Merge Accounts modal window's footer is too high, there might be something weird with modal setup

Fixed in most recent commits.

modal window's title should mention the receiving user for merge, e.g. the one that was initially selected

Done.

all users but the receiving user should be good candidates to be selected for merge, regardless they have linked accounts or not

WIP.

some kind of preview feature before commit is desirable..

WIP.

what about using fontawesome.com/v4.7.0/icon/compress as menu icon?

Sure, done.

I am going to continue with improving the merge process, and follow-up tests. Should be able to finalize by early next week.

[mmoayyed]

Progress report:

• Cleaned up the wizard, and added additional screens/panels for resource selection and review of final linked accounts.
• Clean up panel to demonstrate list of resources available for selection for the target user, turning into a linked-account itself.
• More refactoring and small UI fixes.
• Fix issues with cancel and finish buttons in the wizard.
• Add additional text/summaries to explain intent of panels.

TODOs:

• Item selection via context/action menus should trigger a "next" event to the next panel.
• Disallow navigating to the next panel if previous panel is incomplete state. (select merging user before showing resources, etc)
• Add data to final review panel to preview linked accounts
• Execute merge-account functionality once the wizard is in complete state.
• Clean up tests.

[ilgrosso]

Thanks @mmoayyed for status update; one item is not clear to me:

Item selection via context/action menus should trigger a "next" event to the next panel.

Why? The Admin UI is plenty of wizard steps where a single selection is required to proceed to next step (see Group edit with user owner selection, for example) - yet, nowhere selection triggers wizard's advance.

[mmoayyed]

I was hoping to do this with one click less than expected; For example, when the list of users are displayed one would have to click on a user entry in the list, click select from the action menu, and then click next to advance to the next panel. It would be more ideal to have the select menu trigger a next event automatically, removing the need for the user to additionally click the next button. However, if that behavior is acceptable and is consistent with everywhere else, I can keep it as is.

[ilgrosso]

I understand the proposed improvement; however, I'd drop it for the moment as this PR is already quite considerable.

Anyway, we can always keep if for the future as general improvement, since as said above, there might be other places where the same could be applied.

[mmoayyed]

This should be almost done from a functional point of view. I have added additional panels to show a list of resources, available for selection that would be used for to-be-merged user, and I have also added a panel in the wizard as a final review of all linked accounts. I have also polished the code a bit to handle event sinking for cancel/finish events to handle modal dialog issues as well as the actual merge step.

I'd appreciate a code review, specially on the final merge step to make sure linked accounts are correctly constructed.

What remains is as follows:

1. Have to clean up the integration tests for merged accounts

2. Need to figure out how to add checks for each panel to prevent from advancing to the next step in the wizard if selections (user, resource, etc) are not met. I have not found a good example on this, and did experiment with callbacks such as applyState and isComplete in the wizard, but none is ideal. Is there a good reference on how this might be done? or shall I drop this bit for now, with the assumptions that admin user driving the wizard would make the right selections before moving onto the next step.

[ilgrosso]

@mmoayyed I still haven't find enough time to properly look at actual feature, just browsed the proposed code changes, sorry.

Also, please apply the changes in rebase.txt to rebase this PR with latest changes from 2_1_X.

[mmoayyed]

Thank you for the reviews so far. I will begin to address the feedback and will also rebase.

There is still the matter of finishing integration tests, but I would like to hold off on that for now until the functionality is ready to minimize work as much as possible.

[ilgrosso]

Globally, it works fine, nice! 👍
Just few things as noticed above.

About row selection, a simple improvement could be to highlight the selected row.

[mmoayyed]

I am going to begin to work on tests; all other feedback should be accounted for, and I have also added changes to handle the highlighting of the selected row during the user/resource selection process. (It works, it's not pretty, but I couldn't find any other other approach that would be compatible without breaking changes)

[mmoayyed]

This should be good to go. Sorry for the delay; took me a to organize the tests to merge accounts.

[ilgrosso]

Looks good! Please go ahead, squash and merge.