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

Show which journalist sent a given reply #76

Closed
redshiftzero opened this issue Oct 26, 2018 · 26 comments · Fixed by #1142
Closed

Show which journalist sent a given reply #76

redshiftzero opened this issue Oct 26, 2018 · 26 comments · Fixed by #1142

Comments

@redshiftzero
Copy link
Contributor

redshiftzero commented Oct 26, 2018

Each conversation in the conversation view is between one source and multiple journalists. We are storing this information and should show on the conversation view which journalist sent each reply.

If a first name and last name are set, they should be used instead of a username.

Acceptance Criteria

Given that there is a conversation with at least one reply to a source
When I view that conversation in the SecureDrop client
Then individual replies to the source should be identified by the journalist's real name (if set) or user name (if not set).

Given that there is such a conversation
When I view it in the Source Interface
Then the author of the reply should not be identified.

User Story

As a journalist, I want to know which reply was sent by which journalist so that I can understand the conversation history.

[See the user stories spreadsheet for current prioritization of this functionality.]

Updated by @eloquence on 2019-06-10 to reflect first name / last name functionality and add acceptance criteria.

@ninavizz
Copy link
Member

Static wire demonstrating threading. Sources should NOT see the ID badges, but within the Client journalists should all see their own (upper-right, always; in convo thread when relevant) and their colleague's (in convo threads) ID badges.

Letter in the badge is determined by the first letter of a username, or the person's first name (if/when that separate feature is added). https://drive.google.com/open?id=1GxXbYgQAR7nHlnvXxPFZ_oeA1NmZmf0e

Cheerio!

@redshiftzero redshiftzero added the help wanted Extra attention is needed label Nov 6, 2018
@heartsucker
Copy link
Contributor

My interpretation of this is that we need a server-side API to pull the list of current journalists. This is a minimum requirement for this feature.

Additionally, this would allow journalists to update their display name (see freedomofpress/securedrop#1592) and have that change propagated to the clients.

@redshiftzero
Copy link
Contributor Author

so I added journalist_username and journalist_uuid to the reply JSON objects the API provides for this purpose. The sync behavior as already implemented should add journalist users as they are observed to the journalists table

@eloquence eloquence changed the title show which journalist sent a given reply Show which journalist sent a given reply Jun 10, 2019
@eloquence
Copy link
Member

I've updated the top level comment to reflect the recent addition of first name / last name functionality. Like #411, this is blocked on freedomofpress/securedrop-sdk#87 (SDK support for first names / last names) unless we decide to do a first iteration with just usernames.

The current design spec for the conversation view only reflects display of initials; if we ever want to show more than that (e.g., in a hover state), we'll need a design spec for that, as well.

@eloquence eloquence modified the milestones: 0.2.0beta, 0.2.0alpha Jul 2, 2019
@eloquence
Copy link
Member

eloquence commented Nov 23, 2019

Still unclear whether this will make it to beta. Leaving a note here to make sure we carefully reason through the behavior when user accounts are deleted on the server:

  • How do we want to deal with loss of attribution information on the server?

  • Are there any negative side effects to anticipate due to the use of both IDs and UUIDs on the server, e.g., could there be a situation where a new user account gets an ID of an old one, which would cause historical replies to be incorrectly attributed when they are newly downloaded?

@eloquence
Copy link
Member

(Kept on beta but marked as stretch for now, more likely Post-Beta IMO)

@eloquence eloquence modified the milestones: 0.2.xbeta, Post-Beta Mar 20, 2020
@eloquence
Copy link
Member

As expected, moved to post-beta.

@eloquence eloquence added this to Near Term - SD Workstation in SecureDrop Team Board May 6, 2020
@eloquence eloquence moved this from Near Term - SD Workstation to Next sprint candidates in SecureDrop Team Board Jul 21, 2020
@eloquence
Copy link
Member

This is not quite ready for dev yet, but we've agreed to put our heads together (at least @creviera @eloquence @ninavizz) to finalize the design and implementation plan for a first iteration, as part of the 7/23-8/5 sprint.

@eloquence eloquence moved this from Next sprint candidates to SecureDrop Sprint #55 - 7/23-8/5 in SecureDrop Team Board Jul 23, 2020
@sssoleileraaa sssoleileraaa moved this from SecureDrop Sprint #55 - 7/23-8/5 to In Development in SecureDrop Team Board Jul 29, 2020
@sssoleileraaa
Copy link
Contributor

sssoleileraaa commented Jul 31, 2020

update: working on a spike to see how this can be split into a couple or few prs, looks like we need to make a few backend changes to make sure a journalist for a DraftReply cannot be None and to pass around the logged in user when authentication changes (instead of just whether or not the user is authenticated) so that we can get the name of the sender of a reply.

can't remember if we discussed hover-overs to show the fullname but that would be a useful thing to add. also: should we change the journalist sender icon to red when the reply fails?

@sssoleileraaa
Copy link
Contributor

you can see how it looks like the journalist is None here when we are dealing with a DraftReply:
Screenshot from 2020-07-31 16-46-29

i'm at a good stopping point so i'll see if it's just something silly i was doing when i return

@sssoleileraaa
Copy link
Contributor

I created a separate issue for first handling backend changes: #1133

@eloquence eloquence removed help wanted Extra attention is needed stretch goal labels Aug 5, 2020
@eloquence
Copy link
Member

We (Mickael/Allie/Nina/Erik) discussed the first iteration of this feature in some more detail today. We agreed that the first iteration will only use two colors, the A/1 combination from https://scene.zeplin.io/project/5c807ea562f734bd2756b243/screen/5cba6b2f3367280c23df2be4, and a tooltip for showing the full name. The first iteration should use the A color to identify the currently logged in user (if any) both in the AuthWidget and for replies attributed to that user.

Once that's done, we can resolve this issue, and track further UX changes (more colors, improved hover behavior) in separate tickets.

Still TBD: how to render deleted users in the first iteration.

@eloquence
Copy link
Member

For the 8/20-9/2 sprint, we've committed to landing the backend dependencies for this change, including an SDK release (freedomofpress/securedrop-sdk#127), and to get a draft PR for the reply badges functionality up which may not be fully ready for review yet (e.g., handling of deleted users may still be pending).

@eloquence eloquence moved this from In Development to SecureDrop Sprint #56 - 8/20-9/2 in SecureDrop Team Board Aug 21, 2020
@sssoleileraaa
Copy link
Contributor

@eloquence
Copy link
Member

eloquence commented Sep 17, 2020

(Removing epic from the project board; we are tracking #1142 as part of the current sprint, which may fully resolve this issue as an MVP, or we may split off some follow-up work before we call it ready for release).

@eloquence eloquence removed this from SecureDrop Sprint #59 - 9/17-10/1 in SecureDrop Team Board Sep 17, 2020
@ninavizz
Copy link
Member

ninavizz commented Oct 6, 2020

Woop Woop! @creviera

<svg width="36" height="34" viewBox="0 0 36 34" xmlns="http://www.w3.org/2000/svg"><style>.a{opacity:0.9;}</style><title>  Group 37</title><g fill="none"><g fill="#FFF"><path d="M16.7 3C10.6 9.9 9.7 9.7 6.7 1 9.8 9.7 9.2 10.4 0 8.5 9.2 10.4 9.4 11.2 3.3 18 9.4 11.1 10.2 11.3 13.3 20 10.2 11.3 10.8 10.6 20 12.5 10.8 10.7 10.4 9.9 16.7 3" class="a"/><path d="M36 16.6C26.5 22 24 21.5 18.6 12 24 21.5 23.5 24 14 29.4 23.5 24 26 24.5 31.4 34 26 24.5 26.5 22 36 16.6" class="a"/><path d="M23.5 0C24.8 3.5 24.5 4.2 21 5.5 24.5 4.2 25.2 4.5 26.5 8 25.2 4.5 25.5 3.8 29 2.5 25.5 3.8 24.8 3.5 23.5 0"/><path d="M7.5 22C8.8 25.5 8.5 26.2 5 27.5 8.5 26.2 9.2 26.5 10.5 30 9.2 26.5 9.5 25.8 13 24.5 9.6 25.8 8.9 25.5 7.5 22" class="a"/></g></g></svg>

@sssoleileraaa
Copy link
Contributor

Update

Before

Currently, on main, all journalist replies are the same color, except when they fail to send (then they are red) or fail to decrypt (then they are gray). Here is an example of the current client:

current-without-journalist-badges

After

Here is an example of how the PR that closes this issue (ready for review when things settle down for folks and they have time) shows which journalist sent which message, whether the account exists or not (a deleted accounts show the sparkles/stars svg Nina provided here #76 (comment)) and which replies came from the user who is currently logged in (purple badges):

new-journalist-badges

Followup

I opened a followup discussion issue for us to use as a place to capture ideas around how to display errors now that we have reply badges and use colors to mean more than just whether or not there was an error.

@ninavizz
Copy link
Member

ninavizz commented Oct 7, 2020

@creviera I polished the twinklies a bit so they're less-retro and more refined. More twinkly, less sputnik. It's subtle, and may not be worth the late inclusion into the merge, but FYI. Communicating ephemerality is their purpose, not how much I want to move to Palm Springs and live in a cocktaillian utopia.

<svg width="41" height="36" viewBox="0 0 41 36" xmlns="http://www.w3.org/2000/svg"><style>.a{opacity:0.7;}</style><title>  Twinklies</title><g fill="none"><g fill="#FFF"><path d="M16.7 3C10.6 9.9 9.7 9.7 6.7 1 9.8 9.7 9.2 10.4 0 8.5 9.2 10.4 9.4 11.2 3.3 18 9.4 11.1 10.2 11.3 13.3 20 10.2 11.3 10.8 10.6 20 12.5 10.8 10.7 10.4 9.9 16.7 3" class="a"/><path d="M37.7 13.2C27.4 19.2 24.7 18.5 18.7 8.2 24.7 18.5 24 21.3 13.7 27.3 24 21.3 26.8 21.9 32.7 32.2 26.8 21.9 27.4 19.2 37.7 13.2" opacity="0.5"/><path d="M24.5 16C25.8 19.5 25.5 20.2 22 21.5 25.5 20.2 26.2 20.5 27.5 24 26.2 20.5 26.5 19.8 30 18.5 26.6 19.8 25.9 19.5 24.5 16"/><path d="M23.5 0C24.8 3.5 24.5 4.2 21 5.5 24.5 4.2 25.2 4.5 26.5 8 25.2 4.5 25.5 3.8 29 2.5 25.5 3.8 24.8 3.5 23.5 0" class="a"/><path d="M8.5 7C9.8 10.5 9.5 11.2 6 12.5 9.5 11.2 10.2 11.5 11.5 15 10.2 11.5 10.5 10.8 14 9.5 10.5 10.8 9.8 10.5 8.5 7" opacity="0.8"/><path d="M11.5 24C12.8 27.5 12.5 28.2 9 29.5 12.5 28.2 13.2 28.5 14.5 32 13.2 28.5 13.5 27.8 17 26.5 13.6 27.8 12.9 27.5 11.5 24" opacity="0.6"/></g></g></svg>

@sssoleileraaa
Copy link
Contributor

sssoleileraaa commented Oct 7, 2020

looks great, but just double checking that this is how it is intended to look:

Screenshot from 2020-10-07 15-53-33

and for comparison, this is what it looks like with the original sparkles:

Screenshot from 2020-10-07 15-54-12

@ninavizz
Copy link
Member

ninavizz commented Oct 7, 2020

Yup! Do you have a preference? TY for the screenshots, as always!

@ninavizz
Copy link
Member

ninavizz commented Oct 7, 2020

(oof, wanting that "see attachment" and "cannot decrypt reply" to all be italicized and with the em-dashes...)

@eloquence
Copy link
Member

"see attachment" is just a test message from a source, not a software message.

@sssoleileraaa
Copy link
Contributor

oh yeah those messages might be confusing too because i was using some old screenshot examples. right now "cannot decrypt reply" is italicized ✔️ but no em-dashes.

@sssoleileraaa
Copy link
Contributor

@ninavizz - for now, I prefer leaving it with the original sparkles until we compare the new one with transparency some more with multicolor badges - it's not guaranteed that the badge color will be blue, so the transparency makes it harder to see a distinct sparkle shape with lighter colors.

@ninavizz
Copy link
Member

ninavizz commented Oct 8, 2020

@creviera Is this release of the PR shipping with multiple colors?? I'd thought it was only shipping with the SD Blue? No colors will be too light for the stars, because they need to be high-contrast enough for the letters to be clearly legible. That's been a big "limiting" factor in the colors I've recommended.

@sssoleileraaa
Copy link
Contributor

good point, i think i was over thinking this earlier. now's a good time to update this in the pr actually, and we can get more people testing it this way.

legoktm pushed a commit that referenced this issue Dec 11, 2023
Updated dependencies to clear safety checks
legoktm pushed a commit that referenced this issue Dec 11, 2023
legoktm pushed a commit that referenced this issue Dec 15, 2023
Adds docs on generating test data for APIProxy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants