Add multiple face group merge support

[marc7s]

Changes the MergeFaceGroupsModal to support merging multiple face groups at the same time.

This is something that I have found very tedious, only being able to merge one face group at a time. With this change, the MergeFaceGroupsModal now has two pages, the first where you select the destination face group, and then the second where you select one or more source face groups. It has also been requested in issue #718.

I'm looking for some input for the following questions to make it as intuitive as possible to use:

1. There are two ways of opening the modal, from the People page (/people) or from a single person page (/people/id). With the changes, if you open it from the /people page, you will land on the first page where you select the destination first, and then move to the next page where you can select all the sources. But if you open it from the people/id page, it sends you straight to the second screen where you select the sources, automatically selecting the destination for you based on the id of the person you are currently viewing. Another option is still opening the first page, but having the current person preselected for you, this would require an extra step but work the same as when opening the modal from the /people page, maintaining the uniformity. Thoughts?
2. I currently have the title of the button for opening the modal depend on where you open it from, with places where the destination is already known showing "Merge face", and the others showing "Merge people" to match the title with the current context. I could also revert this, keeping the title the same everywhere again to maintain uniformity, but losing the context based action title. Thoughts?
3. The new functionality has the opposite functionality of the original one, in that if you are on a people/id page and open the modal, that person will be automatically set as the destination, rather than source as it was before. I think the original functionality was more logical in that context, but it made more sense to have the destination first and sources last in the modal, so I don't think it would be a good idea to switch them. Does anyone have a good solution to make this more logical? Right now I think it is still clear what is happening with the new titles and descriptions, but I think the flow of operations is not really what you would expect when pressing the "Merge face" button.
4. I have modified the title of the sources screen to include the name of the selected destination, to improve intuitivity. The images below displays the original and new titles, adding the name of the destination person in the title. Thoughts?

Original:

New:

TODO: I was having an issue where sometimes the image faces wouldn't be deleted correctly, leaving two duplicate entries in the database, causing the same image to show up twice on a /people/id page, also causing a React error since the IDs wouldn't be unique. I wasn't able to reproduce it during my latest tests, but I will need to test this more thoroughly to confirm whether it is still an issue or not.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 25.15%. Comparing base (8cdc4cd) to head (a6e39d6).
Report is 15 commits behind head on master.

Files	Patch %	Lines
api/graphql/resolvers/faces.go	0.00%	17 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #912       +/-   ##
===========================================
- Coverage   36.18%   25.15%   -11.03%     
===========================================
  Files          38       96       +58     
  Lines        1708     4412     +2704     
===========================================
+ Hits          618     1110      +492     
- Misses        934     3053     +2119     
- Partials      156      249       +93     

Flag	Coverage Δ
api	25.15% <0.00%> (-11.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[marc7s]

I added a button for opening the MergeFaceGroupsModal from the People page as well, and not only the single person page as the modal now supports selecting the destination and sources through the modal. If I get the initial value working I'm planning on setting it to the current face you are viewing if opening the modal from a single person page (perhaps just setting it for you and opening the modal in the sources mode?), and leaving it unselected when opening the modal from the People page, so you have to set the destination manually.

I also added another translation entry for this button, to be more in line with the English translation that is "Merge face" instead of "Merge people", so it is now split so the button on the People page says "Merge people", but the button on the single person page says "Merge face", which I think reflects well what the action is doing based on the context. For the missing translations, I think it could be in line with the English translation where the button on the People page is in plural ("Merge people") and the button on the single person page is in singular ("Merge face"). Maybe the destinction between faces and people is also good, I think it makes sense based on the context: Merge people/Merge face rather than Merge faces/Merge face.

[marc7s]

I managed to fix the issue I was having earlier so now if the preselected destination is set, it will skip the destination screen of the modal and send you straight to selecting the sources, with the preselected destination as the destination.

I also updated the title to make it clear what you are doing, explicitly naming the destination label of the merge (see the images in the description)

[kkovaletp]

@marc7s, thanks for preparing this PR and doing all this hard work of research, design, and implementation!
I'll provide my thoughts here chronologically to your original posts while reading them:

1. There are two ways of opening the modal, from the People page (/people) or from a single person page (/people/id). With the changes, if you open it from the /people page, you will land on the first page where you select the destination first, and then move to the next page where you can select all the sources. But if you open it from the people/id page, it sends you straight to the second screen where you select the sources, automatically selecting the destination for you based on the id of the person you are currently viewing. Another option is still opening the first page, but having the current person preselected for you, this would require an extra step but work the same as when opening the modal from the /people page, maintaining the uniformity. Thoughts?

I'd vote for the 2nd option, as the usual scenario for me is to go to some newly created persons to make sure that I recognize them correctly and merge them to the destination person, already existing in the DB. If I have to do all the person management from scratch, of course, option 1 looks more natural, but if I already have some persons managed in the DB and after uploading new photos have incorrectly recognized persons leading to duplicates creation, I'll go to those duplicates to figure out the recognition results and merge them to some of the existing destinations.

2. I currently have the title of the button for opening the modal depend on where you open it from, with places where the destination is already known showing "Merge face", and the others showing "Merge people" to match the title with the current context.

Having different button names looks natural for me even with my scenario, described earlier, because I still want to merge the particular face when opening the page from a person. The difference is how this person is tracked: source or destination.

3. The new functionality has the opposite functionality of the original one, in that if you are on a people/id page and open the modal, that person will be automatically set as the destination, rather than source as it was before.

Well, both workflows might work. I think that it is not that important as far as it is absolutely clear from a UX perspective what is going on and what is expected. Just take into account different user scenarios. Also, as an alternative solution, you might introduce a new option on the Settings page, which will define which direction to use (having by default the original behavior).

4. I have modified the title of the sources screen to include the name of the selected destination, to improve intuitivity.

Nice UX improvement! However, if there will be a different direction of the workflow or both directions supported (with the option on the Settings page), you'll need to handle the case, when there is no destination defined yet: not to show the last part of the text.

I added a button for opening the MergeFaceGroupsModal from the People page as well, and not only the single person page as the modal now supports selecting the destination and sources through the modal.

Nice, just make sure it is consistent with all the supported scenarios, described earlier.

I also added another translation entry for this button, to be more in line with the English translation that is "Merge face" instead of "Merge people", so it is now split so the button on the People page says "Merge people", but the button on the single person page says "Merge face", which I think reflects well what the action is doing based on the context. For the missing translations, I think it could be in line with the English translation where the button on the People page is in plural ("Merge people") and the button on the single person page is in singular ("Merge face"). Maybe the destinction between faces and people is also good, I think it makes sense based on the context: Merge people/Merge face rather than Merge faces/Merge face.

Great! If speak about wording, it looks to me that you're mixing things together: if we use person as an object, it would be more consistent to use the same term: person and people; if in opposite, we use face as an object, then let's name that face and faces. Of course, this is not related to the code and internal product architecture, but only to the User eXperience.
We have a list of translations in the app and a few more in PRs. Is it clear how to update them to support your new addition? will it fail back to English in the case that the current translation doesn't have it translated? Maybe it makes sense to consistently add all the structure and at least English wording of the new addition to all translations, already present in the code (this depends on the answer to my previous question, I see some changes in translations, but I noticed more changes in English file comparing to others)?

From the code perspective, this PR introduces significant changes and potentially can bring some bugs to the product. This is great that you have tested the implementation, but I think that it is crucial to also cover all the changes by unit tests on the component level and add some end-to-end scenarios (at least 1 per workflow) to make sure everything works in the designed way. This is even more important, while the project is on hold and I'm not a developer, so I cannot do a code review. I see the Codecov report with the line "All modified and coverable lines are covered by tests", but please double-check that everything is covered from a logic perspective including e2e tests.

I hope that you know about the #910 PR, which is an alternative to the master while the project is on hold (if not yet - feel free to check it). When this PR is ready, I'll merge it there so others can use it as well.

[marc7s]

Hi @kkovaletp and thanks for the feedback!

I'd vote for the 2nd option, as the usual scenario for me is to go to some newly created persons to make sure that I recognize them correctly and merge them to the destination person, already existing in the DB. If I have to do all the person management from scratch, of course, option 1 looks more natural, but if I already have some persons managed in the DB and after uploading new photos have incorrectly recognized persons leading to duplicates creation, I'll go to those duplicates to figure out the recognition results and merge them to some of the existing destinations.

Sure, that makes sense. I can just switch it so it preselects them but doesn't open the source pane immediately.

Having different button names looks natural for me even with my scenario, described earlier, because I still want to merge the particular face when opening the page from a person. The difference is how this person is tracked: source or destination.

I'll keep it as is then.

Well, both workflows might work. I think that it is not that important as far as it is absolutely clear from a UX perspective what is going on and what is expected. Just take into account different user scenarios. Also, as an alternative solution, you might introduce a new option on the Settings page, which will define which direction to use (having by default the original behavior).

Yes I agree, that's why I tried to make it clear through the clarifications in the UX what is happening, to make the merging like a story or guide to help you along the way, but UX is not my strong suit so I'm happy for any and all feedback.

Nice UX improvement! However, if there will be a different direction of the workflow or both directions supported (with the option on the Settings page), you'll need to handle the case, when there is no destination defined yet: not to show the last part of the text.

Absolutely, it would be really easy to not show that last part in that case. However, I am not sure how to effectively support the other direction. The issue is that the source selection is one or multiple, whereas the destination must be a single face. So you could select multiple sources in the first panel (if it switches to that direction), however you would then need to select the destination on the next page, and I would need to filter out all the sources you have selected previously because you cannot of course have the destination be part of the source selection. So you would have to leave one out for the source selection first, and select it in the second screen. I mean it might be reasonable, I just think it might make the UX more complicated to support both options and I think it might just be confusing having a button or something that lets you switch directions. My thinking is that you can still perform all the possible actions from this direction, so even if it might be a bit more clunky in some cases where you are looking to do the opposite (you know that the current one should be merged into another face, so you would like to go source -> destination) I think it clears up the UX enough to leave it as just a forced direction. I'm open to discussions on the topic, I'd just like to share my point of view, in this case a "less is more" approach and simplifying it by limiting the user to make it easier to understand.

Great! If speak about wording, it looks to me that you're mixing things together: if we use person as an object, it would be more consistent to use the same term: person and people; if in opposite, we use face as an object, then let's name that face and faces. Of course, this is not related to the code and internal product architecture, but only to the User eXperience. We have a list of translations in the app and a few more in PRs. Is it clear how to update them to support your new addition? will it fail back to English in the case that the current translation doesn't have it translated? Maybe it makes sense to consistently add all the structure and at least English wording of the new addition to all translations, already present in the code (this depends on the answer to my previous question, I see some changes in translations, but I noticed more changes in English file comparing to others)?

A few answers on this question.

1. Yes I agree, however I have tried to follow the current standard in this PR. However, I think it would be a good idea to open another issue about the consistency, because currently it is not consistent, especially not between languages. Sometimes English uses person/people, but a translation will use face/faces. So I think we should stick to one as you say, and then update the translations to reflect that. However, I don't think that change should be made in this PR, but in a dedicated issue
2. Yes, all translations fall back to English if they are missing. To be precise, they fall back to the provided English text in the code and not the English translation from the language files, but they match most of the time unless someone forgot to update the text in the code when changing the translation
3. Yes, it is clear how to update them. I added the new translation structure to all languages (some new fields that need to be translated), so it is just a matter of filling in the blanks like it is currently. You can see the structure update in the commit 0ca5e10.
4. Yes, I speak English and Swedish so I can translate those, so I added the relevant translations there which is why you see them filled in for the parts I touched, but all files should have the same structure, just that other languages are missing the translation (and will therefore fallback to English)

From the code perspective, this PR introduces significant changes and potentially can bring some bugs to the product. This is great that you have tested the implementation, but I think that it is crucial to also cover all the changes by unit tests on the component level and add some end-to-end scenarios (at least 1 per workflow) to make sure everything works in the designed way. This is even more important, while the project is on hold and I'm not a developer, so I cannot do a code review. I see the Codecov report with the line "All modified and coverable lines are covered by tests", but please double-check that everything is covered from a logic perspective including e2e tests.

Yes this is something I would like to add, however I'm not overly experienced with writing tests, and this is my first experience with the entire codebase including some of the programming languages and frameworks, so it has been quite overwhelming sometimes. I will look into writing additional tests, but I have currently not written any tests for this feature and that should be added as you mentioned. There are a bunch of code coverage metrics, of which line coverage (as you saw reported by Codecov) is one, but not the best. So even though it seems like this PR does not lower the line coverage (according to the bot), it certainly lowers the branch coverage since there are no tests for these added workflows. On the topic, as I mentioned in the TODO, I still need to check if the bug I found is still present or not also.

I hope that you know about the #910 PR, which is an alternative to the master while the project is on hold (if not yet - feel free to check it). When this PR is ready, I'll merge it there so others can use it as well.

Yes I have seen your PR, which is one of the main reasons I started to work on this feature since I am using a build from your PR and felt this was a missing feature (as did others in the associated issue). I have a few other things that I also feel are missing, that I would like to move on to after this if I have the time. Thanks for all the work on that PR and in the Discord!

[kkovaletp]

@marc7s,

I am not sure how to effectively support the other direction. The issue is that the source selection is one or multiple, whereas the destination must be a single face. So you could select multiple sources in the first panel (if it switches to that direction), however you would then need to select the destination on the next page, and I would need to filter out all the sources you have selected previously because you cannot of course have the destination be part of the source selection.

It might be as easy, as filtering out on 2nd step all people, selected on the 1st one, no matter which it was: source or destination. Then you can define another limitation also independently to step order, but for step purposes: source --> multi-select, destination --> single-select. In this way, you can keep code simple and handle the logic independently to the order of steps.

I think it would be a good idea to open another issue about the consistency, because currently it is not consistent, especially not between languages.

Sure, please open a new issue, so later you or someone else can take care of it. Please add all the details, so a person not in the context would easily understand what is wrong and how to fix it.

I'm not overly experienced with writing tests, and this is my first experience with the entire codebase including some of the programming languages and frameworks, so it has been quite overwhelming sometimes. I will look into writing additional tests, but I have currently not written any tests for this feature and that should be added as you mentioned.

Sure, take your time to try and learn unit testing concept and different approaches. 1 of the big advantages of open-source projects is the ability to try and learn new things. Just switch the PR to the Draft state and make sure everything is done and covered before switching it back to Ready. Feel free to ask questions here or in Discord - there is a chance that you'll get an answer or at least some direction for the research.

[marc7s]

@kkovaletp,

I have changed the behaviour to only preselect the destination, and not open it straight in the sources pane when the destination is known.

It might be as easy, as filtering out on 2nd step all people, selected on the 1st one, no matter which it was: source or destination. Then you can define another limitation also independently to step order, but for step purposes: source --> multi-select, destination --> single-select. In this way, you can keep code simple and handle the logic independently to the order of steps.

Sure, but the logic part of it is not the difficult part. The source: multi-select and destination: single-select is already how it is implemented. I would only need to change some details to support both directions in code, however what I find difficult is how to present it in the user interface. I feel it would be overly complicated and cluttered with even more buttons on the panel, and that I don't really feel a "multi-select but avoid clicking one of them -> single-select the avoided one" workflow is very intuitive or clean, it just feels more natural to do "single-select -> multi-select all the rest" as it is now. But I am open to ideas, so if you can figure out a way to cleanly integrate both directions from a UX perspective, and you feel that it is needed to be able to go in both directions then I could add that.

Sure, please open a new issue, so later you or someone else can take care of it. Please add all the details, so a person not in the context would easily understand what is wrong and how to fix it.

I have opened #914.

Sure, take your time to try and learn unit testing concept and different approaches. 1 of the big advantages of open-source projects is the ability to try and learn new things. Just switch the PR to the Draft state and make sure everything is done and covered before switching it back to Ready. Feel free to ask questions here or in Discord - there is a chance that you'll get an answer or at least some direction for the research.

I have an understanding for the concepts and different types of testing, but I haven't coded in many of the used programming languages and frameworks so I will need to get familiar with the testing frameworks used and so on before I can write the required test cases. I've switched it to draft while I work on this.

Thanks again for the feedback!

[kkovaletp]

what I find difficult is how to present it in the user interface. I feel it would be overly complicated and cluttered with even more buttons on the panel, and that I don't really feel a "multi-select but avoid clicking one of them -> single-select the avoided one" workflow is very intuitive or clean, it just feels more natural to do "single-select -> multi-select all the rest" as it is now.

I don't insist on the 2-way implementation, just provided my thoughts... If you feel that the implemented way looks more clear and natural for users, while adding this flexibility would noticeably decrease usability, then sure, let's go with the current implementation and see how it will work.