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

Fix issues with the group sync with "Replace" strategy #557

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

wynnset
Copy link
Contributor

@wynnset wynnset commented Feb 6, 2018

Fix issues with the group sync when choosing "Replace group rather than merge" flag

  • Update both import and export logic and also do some refactoring
  • For import, ensure Canvas group is seen as empty if it contains only ineligible members (i.e. members not in iPeer)
  • For import, fix an issue with merging the imported non-empty groups with imported empty groups
  • For export, if update flag is on, delete all the groups in canvas right away to allow users that are in other groups to be exported (if in only 1 iPeer group)
  • For export, check group memberships when exporting each group (mainly code refactoring)
  • Fix an issue when sync was not working as expected when group names are numeric
  • Improve error messages

Closes: #556

@wynnset wynnset self-assigned this Feb 6, 2018
@wynnset wynnset requested a review from kitsook February 6, 2018 21:44
Copy link
Contributor

@kitsook kitsook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Notice one thing while testing but not related to this issue.

Canvas does allow groups with same name while iPeer doesn't. The chance for this to happen should be quite low but it may affect the sync result.

@wynnset wynnset force-pushed the #556-group-sync-replace-fix branch from 1138241 to d170d4f Compare February 7, 2018 02:14
@wynnset
Copy link
Contributor Author

wynnset commented Feb 7, 2018

@kitsook good call. I added in a check for that and it lets the user know they need to change their Canvas groups to have unique names before being able to sync (disables sync meanwhile).

…an merge" flag

- Update both import and export logic and also do some refactoring
- For import, ensure Canvas group is seen as empty if it contains only ineligible members (i.e. members not in iPeer)
- For import, fix an issue with merging the imported non-empty groups with imported empty groups
- For export, if update flag is on, delete all the groups in canvas right away to allow users that are in other groups to be exported (if in only 1 iPeer group)
- For export, check group memberships when exporting each group (mainly code refactoring)
- Fix an issue when sync was not working as expected when group names are numeric
- Improve error messages
@wynnset wynnset force-pushed the #556-group-sync-replace-fix branch from d170d4f to 6389d84 Compare February 7, 2018 20:14
@wynnset
Copy link
Contributor Author

wynnset commented Feb 9, 2018

@xcompass this is ready to be merged in. Feel free to either merge it in or let me know if you'd like me to make any other changes.

@wynnset wynnset assigned xcompass and unassigned wynnset Feb 9, 2018
@wynnset wynnset added this to the 3.3.1 milestone Feb 15, 2018
@xcompass xcompass merged commit d9d941b into master Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group Sync with Canvas Not Working Properly
3 participants