-
Notifications
You must be signed in to change notification settings - Fork 920
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
Replace submitted note table colors with created/resolved #5269
base: master
Are you sure you want to change the base?
Replace submitted note table colors with created/resolved #5269
Conversation
You say you ignore reopen events but in fact this seems to extend the concept of submitted/created from who initially opened it to include anybody that reopens it as well? You are considered to have created it if there are any open events by you. Obviously that is mirrored in terms of closing by looking for any close event, but aren't users going to be more interested in whether the note is currently open or closed and whether this person closed it? |
Are you proposing to treat reopen events exactly like initial open events? That's not what we're currently doing with the blue submitted coloring. It only looks at the initial open event, or actually it does the equivalent thing of checking the author (the user who did the initial opening). |
After your change the test is: opened_by_user = note.comments.any? { |comment| comment.author == @user && comment.event == "opened" } which will be true if the user has ever opened it, either the original open or a reopen? |
This is a test only for original open events:
Reopen events are stored as |
Another option is to ignore all close events unless it's the last event on the note, that's symmetrical to ignoring reopen events. |
Oh sorry I looked at the source last night and was quite sure they were all marked as opened. |
Why check every comment though when the initial open will always be the first one? There's still a question about resolution I think? Should we be looking at every one or just the last one? |
There are some drawbacks in looking only at the last close event. #5255 tried to do filtering by interaction type but the db queries are ugly. To optimize them we could have a users x notes table where we store among other things interaction flags. When a user closed a note, we set a closed flag there, then we can filter notes by that flag. But if we want to pay attention only to the last close event, we'd have to clear that flag when somebody reopens the note. |
My thinking was literally just to look at the last event which should be the close event unless it's been reopened and if it has then do we want to consider it closed by this user? |
4f4212a
to
7aeafbd
Compare
Alright, let's not think about filtering for now. Changed to look only at the last close event. I also wanted the "commented" indication not to be misleading. It includes more than commenting and this change stretches it further by including non-last close events. So I removed the claim that not colored notes were commented, although the sentence gets a bit long: |
7aeafbd
to
837aeea
Compare
Instead of submitted and commented note colors switch to created, resolved and commented.
Reasons:
Caveats:
subheading_html
is not updated.Dark mode:
Legend while locale strings aren't updated: