habitica
habitica copied to clipboard
HabitRPG
Failed party invitations do not indicate which invitations failed
General Info
- UUID: 30983c37-52c3-415d-977c-38af6c7db91c
Description
Party, Invite Friends, Invite Existing Users. Invitations can be sent to multiple users via the '+' button. However, if any invitation should fail, the standard message User already in a party pops up. The standard message does not indicate which user(s) invitation failed (which user is already in a party) nor does it indicate how many invitations failed (I tested this by sending invitations to two users who are already in other parties, and only one failure message popped up).
edit by Alys 2018-02-20:
PR #9939 has done a partial fix for this and we now see a single error message if one invitation has failed. However if two or more have failed, only the first error is reported. See the comments in this issue for details.
Some of the comments mention using bluebird but we're moving away from it so best to not use it for this issue.
Some of the comments below will be related to the partial fix that was done in #9939 so they won't be relevant to the remaining work. Post here if you aren't sure about any comments!
@DrStrangepork So, are you looking for something more like " User name already in party"?
Sorry to jump in here if I am not supposed to. CS student here, trying to work on first contribution to project - as part of an open source class.
That's actually a good question, because the object of the invitation is the UUID, so if it fails, it would be the most consistent of the response was User <UUID> already in a party. However that would make the popup significantly larger than current, ~2x the width. Not sure we want that. The alternative would be "name".
The other question would be how to approach multiple failed invitations? Would we prefer: User name1 already in party User name2 already in party
or
Users already in party: name1 name2 (Name and UUID interchangable)
I would rather see the first version. Also, maybe, showing the last few chars of the UUID instead of the full string (if name is decided against)
How about something like this:
Alys (d904bd62-da08-416b-a816-ba797c9ee265) is already in a party
I don't think it matters that the notification will be larger because the information is important.
I think it would be easiest for the message to be repeated in full for each person it applies to (Dagrik's first version), since the code could then issue a message as soon as it finds each unavailable user, instead of having to store them all and print them all together. It would also be better for any third-party developer who was using the API to issue invitations since they could programmatically handle the error messages more easily if each unavailable user had their own message.
If you two believe this could be an easier fix, is this something I could work on to get my feet wet? Currently, I see using the UUID to request the name, then output them both with the alert. It is just a matter of trying to track the path of the alert, then ensuring unavail found provides the message.
@Dagrik Yes, I think this should be pretty easy. I'm marking it as in progress by you. If you run into trouble or have any questions, comment here. Thank you!
As I was trying to find the locations of the messages, and trying to track where code may have to be altered, I noticed other messages such as if multiple invitations are sent and more than one UUID doesn't exist, still only one message is displayed with the first bad UUID. Is the goal to fix that across the board?
Progress: Got the message to display "User name (UUID) already in a party". Now I just have to find where in the code we do the inviting from, so I can work on displaying messages per each uuid input. Thinking just running the code with a for..in loop, but I won't know until I can find the code.
@Alys I could use some guidance. Got the changes done where the name and uuid display on the "user already in a party" message. Just to take a step further, also added userID to user already pending invitation, and displays correctly. I know it is completely cycling through the uuid list (placing one "bad" uuid randomly in a list for testing). However it won't display a second or third error, even though it will finish the list of UUIDs and send all the invites. Can you point me in the right direction of where the UUIDs are cycled/looped through?
Edit: Additionally, the uuid "spills" outside the error box in firefox, but not in chrome.
Don't worry too much about the spilling. We're part way through a project to improve the website's front-end code so the way the notifications are displayed is likely to change.
I think you must have already found most of the code you need if you've been able to display the name and ID. You would have modified the userAlreadyInAParty locales string and the website/server/controllers/api-v3/groups.js file, right? That file is the right place to look. The NotAuthorized error that uses userAlreadyInAParty is in the function called _inviteByUUID, so search for where that function is called - the relevant place is about line 1097 in the same file. uuids.map does the looping.
As to why the second, etc errors don't display, I'm not sure. :(
Could it be that in throwing errors, as opposed to loading all the strings for later display, we are losing the ability to continue?
Edit: I did some testing in JSFiddle with try/catch statements. The first error displayed fine, and other good values did good. However, it only displayed one error. If I added another faulty value, only the last error would display.
@Dagrik Thinking about this more, I don't think that's it. One failing user doesn't stop the others being invited. It's likely to be because when you make an HTTP request, there can be only one status code and message returned. When you do multiple invitations, they're all sent in a single HTTP request, and so you get back only one message, no matter how many failures occur.
This means that this isn't actually an easy fix after all - I'm sorry for misleading you. We'd need to modify the _inviteByUUID function so that instead of throwing an error as soon as it finds one, it sends the error message back, and then when the calling function has processed all the invitations, it looks at whether it's been sent any error messages, and if so makes the overall result an error code instead of a success code, and then shows all of the error messages at once.
This would make the error handling less neat in the code, but more useful to the end user. As DrStrangepork says in this issue, it would be ideal to show the user all of the errors that occurred, and that could be one or more of several different types of errors (cannotInviteSelfToGroup, userAlreadyInGroup, userAlreadyInvitedToGroup, userAlreadyPendingInvitation, userAlreadyInAParty). At the moment, if two or more errors occur, the user can never learn about more than one of them.
So, I think this issue is definitely worth fixing, but it's not a quick fix.
We should wait for other admins to comment with their opinions before anyone starts writing code.
Okay. The only written code thus far is to fix the errors to show UUIDs and names.
So, being one of the newest (potential) contributer, I like your idea @Alys. Error messages get added to an array/linked list/ array of structs/etc, then if not empty, we loop through displaying each message, and throw one error.
Again, not an admin, or an experienced contributer. I can't speak for how it would affect other code, or the amount of work required to integrate and/or test said change. But just trying to contribute in a way I know...
The idea is correct! Here is a more lower level flow:
- Use Bluebird.all to process all promises
- Loop through return values and check for errors
- Return human readable error lists
@Dagrik If you'd like to go ahead following TheHollidayInn's flow, please do! Tell us if you have questions or need more details.
We're aware that this is now more complex than I originally thought (my mistake - so sorry!) so if you'd prefer someone to take over now, that's also okay. The work you've done so far might be usable in its current form to at least slightly improve the present situation so if you want to submit a PR with your changes so far to let us look over them, go ahead.
@Alys I will get the changes thus far submitted this weekend for review - maybe to at least get part of the original problem fixed. I want to read up on bluebird first, as I am unfamiliar with it. I will check it out tomorrow, and let you know - but don't count me out yet!
Still on my todo list, just school, fam, and a new house had to take priority. Just having an issue with making the test display a name.
On Oct 20, 2017 10:31 AM, "MathWhiz" [email protected] wrote:
Note: This has not been fixed in the redesign
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HabitRPG/habitica/issues/8735#issuecomment-338223530, or mute the thread https://github.com/notifications/unsubscribe-auth/ATHuZndVIoKxgGaQKE4XJ7q_lSREFFlvks5suK6xgaJpZM4Nd-tm .
@Dagrik I saw you deleted your PR--are you planning to spin up a new one, or should this issue be released for someone else to tackle?
I didn't realize that got deleted. Oops. Well, I will be spinning up a new one. Literally (as can be seen above), I was just having an issue with getting the test changed to see the name.
@lemoness Just finishing right now and running thru tests. Just finished final exams week.
I'm reopening this because #9939 says it's only a partial fix. The PR works well to display a single error message (which I believe was the only intention for that PR) but for cases where there's errors with two or more invited players, only one error is shown. There's a lot of discussion about that in this issue, so I think we should keep going with this issue rather than create a new one. I'll edit the top post to describe what this is up to.
I had almost forgotten about the other part here. I will revisit @TheHollidayInn 's lower level view, and have to review bluebird first, and will give this a shot after that.
Thanks very much, Josh! I'm not sure if we're still using bluebird. Might be worth checking before making plans.