Add feature of tagging users in photos

[wilco375]

Summary

Add ability to tag other users. Related to #538, however tagging is still manual for now. As Elise mentioned in her mail, might be a good SOG activity to tag people.

When clicking on the image, you can tag a user:

Tagged users are shown after adding a user or clicking the button at the top-right:

In the user's profile, you can see all images in which the user is tagged:

You can only delete tags that you have created or which tag you (i.e. remove tags which incorrectly tag you)

Requires: csvalpha/amber-api#415

[DrumsnChocolate]

I'm sorry I have not had time to look at this yet. Do you know if this works well on mobile?

[wilco375]

I'm sorry I have not had time to look at this yet. Do you know if this works well on mobile?

It's not great if there are a lot of people in the picture or near the edge, but it does work. Tagging is most easily performed on a desktop device.

[DrumsnChocolate]

Okay, testing this feature leads to the following comments:

• I'd like the input box to disappear when I press escape without selecting a name, or when I click next to the picture. On mobile, when the input box is visible, clicking anywhere next to the input box should hide the input box (instead of moving the input box to a new place).
• Leaving the route (or changing the route parameters) should hide the input box.
• I like that you place the input box exactly where the click is placed, but this may take a bit of getting used to for users. It is easiest for us to implement, and allows the user full control over where the tag is placed. Control is not necessarily the same as ease of use, since you kind of want the tag to be placed slightly offset from the thing you're tagging, but it's a good compromise. A possible improvement would be making the input box draggable, but that's definitely overengineering.
• the input box can appear in front of the button that reveals existing tags. This might not be optimal (unless it's easy to hide the input box, see first remark.)
  
• Can we make tags link to the profile of the tagged person? Such that we can easily navigate from a picture to the users tagged in the picture?
• I can imagine people who don't want to be tagged in pictures at all. Should we add a 'privacy' setting that enables a user to exclude themselves from the list of taggable people?

code related comments will follow later, either Monday evening or Friday evening

[wilco375]

Thanks Matteo.

• I have adjusted the code so that the tag box is hidden when clicking outside it or leaving the route. I could not get the escape key working unfortunately.
• Leaving the route should have already hidden the input box since the component is then no longer rendered. This seems to be working for me. Did you have any problems with that?
• I am failing to understand if you want anything changed on the behavior / methodology of the tagging 😅
• It's now easier to hide.
• Added. I linked to the overview of tagged photos
• Personally I feel like this isn't necessary, but I'd like to hear the opinion of other committee members on this.

[DrumsnChocolate]

Looks great overall. I have made some remarks which I will implement tommorow (unless you object or would like to take care of the suggestions yourself). I have focused on verifying whether the code was Ember Octane compliant. I have absolutely skipped the css.

[DrumsnChocolate]

https://guides.emberjs.com/release/upgrading/current-edition/action-on-and-fn/ the action helper is getting deprecated in favor of @action (which can be used in js files) paired with the helpers {{on}} or {{fn}} (which can be used in hbs files). I see you used it in other places, did you have a reason not to do so here? In these cases specifically, I suspect we may be able to do something like:

Suggested change

	@options={{this.users}}
	@onChange={{action 'storeTag'}}
	@searchEnabled={{true}}
	@searchField='fullName'
	@registerAPI={{ action 'openUserSelect' }}
	@options={{this.users}}
	@onChange={{this.storeTag}}
	@searchEnabled={{true}}
	@searchField='fullName'
	@registerAPI={{this.openUserSelect}}

Though I will of course check whether this indeed works.
And I'm not entirely certain this is precisely what we should do (what about using {{on 'change' this.storeTag}} instead of my proposed @onChange={{this.storeTag}}? Would that work? )

[DrumsnChocolate]

while applying this, I noticed that the powerselect includes users that have already been tagged. Then an error message is shown if you try to tag that user, which I like a lot. It makes sense because people may not know that the user is already tagged, and this informs them about it!
However, next step from the tagger's perspective in the case of a wrong location of the existing tag, is to correct its location. That's not directly possible with the current way permissions are setup, but they can I guess inform the user that's being tagged that the tag is incorrect, and then the tagged can remove the tag. In case of an inactive user, that is not really workable, and the person trying to tag needs to inform someone who has deletion rights (ICT for example). It's not an amazing situation to occur, but it may be rare, and I don't see an easy fix.

[DrumsnChocolate]

{{on 'change' this.storeTag}} seems to not work, console complains that PowerSelect requires an @onChange function

Not sure what this means for when we actually finish the transition to octane, but I'm also not going to care right now

[DrumsnChocolate]

nice 👌

[DrumsnChocolate]

I want to verify whether we can use {{@tabItems}} or {{this.tabItems}} here. I belive tabItems originates at the user route, so I'm not sure? this is for controller originated properties, and @ is for named arguments, so we'll see if route properties are just supposed to be 'bare'

[DrumsnChocolate]

this.tabItems seems to work fine, so I'm also applying it for the other tab routes under app/templates/users/user/ (i.e. groups, mail, mandates, etc.)

[DrumsnChocolate]

I'm still not entirely sure why this works? or what the rules are about properties defined on routes.
I've thought about it some more, and in principle the tabItems and pageActions properties should ideally not be coming from the route, but from the controller/component js file that belongs to a template. (I think that means we are supposed to pass any relevant info from the route to the controller in order to allow the controller to provide these properties) But that's for another PR with a focus on refactors, rather than a new feature.

[DrumsnChocolate]

And I'm sorry for how long you've had to wait for a review, this is honestly the first time since March that I've actually had time to work on this. graduation took some time, and then a summer break got in the way

[wilco375]

Thanks for the review Matteo! And no problem, I understand you were busy 😉 I have a busy weekend coming up so I'll likely double check and then merge/deploy everything at the beginning of next week!