pleroma/pleroma-fe
Watch
2
2
Fork
You've already forked pleroma-fe
 10
Code Issues 351 Pull requests 26 Projects Releases 5 Packages 1 Wiki Activity

Prevent showing pinned statuses twice #2081

Closed
tae wants to merge 0 commits from gitlab-mr-iid-812 into develop
pull from: gitlab-mr-iid-812
merge into: pleroma:develop
Conversation 32 Commits - Files changed -
tae commented 2019-05-26 18:19:09 +00:00
Member
Copy link

closes #549

closes #549
hj commented 2019-05-26 18:26:53 +00:00
Member
Copy link

Not liking the whole idea. If pinned post is 3 weeks ago it should still be visible there in main timeline. Not displaying same post twice in a row however is good, or same sequence of posts.

Not liking the whole idea. If pinned post is 3 weeks ago it should still be visible there in main timeline. Not displaying same post twice in a row however is good, or same sequence of posts.
tae commented 2019-05-26 18:31:47 +00:00
Author
Member
Copy link

We fetch and show all pinned posts at top of the timeline, and this behavior is limited to the user timeline.

We fetch and show **all** pinned posts at top of the timeline, and this behavior is limited to the user timeline.
hj commented 2019-05-26 21:07:53 +00:00
Member
Copy link

and?

and?
hj commented 2019-05-26 21:19:59 +00:00
Member
Copy link

What i'm saying is -
This is bad:

Post 1 [pinned]
Post 2 [pinned]
Post 1 [pinned]
Post 2 [pinned]
Post 3
Post 4

This is also bad:

Post 3 [pinned]
Post 1
Post 2
Post 4

What it should look like:

Post 1 [pinned]
Post 2 [pinned]
Post 3
Post 4

Post 3 [pinned]
Post 1
Post 2
Post 3 [pinned]
Post 4
What i'm saying is - This is bad: ``` Post 1 [pinned] Post 2 [pinned] Post 1 [pinned] Post 2 [pinned] Post 3 Post 4 ``` This is also bad: ``` Post 3 [pinned] Post 1 Post 2 Post 4 ``` What it should look like: ``` Post 1 [pinned] Post 2 [pinned] Post 3 Post 4 ``` ``` Post 3 [pinned] Post 1 Post 2 Post 3 [pinned] Post 4 ```
tae commented 2019-05-27 01:20:58 +00:00
Author
Member
Copy link

Well, I've compared it with Mastodon FE. Mastodon FE uses the second one.

Post 3 [pinned]
Post 1
Post 2
Post 4

I would like to know your thoughts @lambadalambda @shpuld @feld

Well, I've compared it with Mastodon FE. Mastodon FE uses the second one. ``` Post 3 [pinned] Post 1 Post 2 Post 4 ``` I would like to know your thoughts @lambadalambda @shpuld @feld
feld commented 2019-05-28 17:37:18 +00:00
Owner
Copy link

We should implement the same behavior as Twitter. How do they prevent duplicate pinned posts?

We should implement the same behavior as Twitter. How do they prevent duplicate pinned posts?
tae commented 2019-05-28 17:42:28 +00:00
Author
Member
Copy link

Twitter uses the second one.

Post 3 [pinned]
Post 1
Post 2
Post 4
Twitter uses the second one. ``` Post 3 [pinned] Post 1 Post 2 Post 4 ```
shpuld commented 2019-05-29 17:06:23 +00:00
Member
Copy link

seeing a variable called this raises some red flags in me, to me it'd be less alarming if it clearly indicated it's pinned statuses and not some other exclusion mechanism that should probably exist on status level instead of timeline

seeing a variable called this raises some red flags in me, to me it'd be less alarming if it clearly indicated it's pinned statuses and not some other exclusion mechanism that should probably exist on status level instead of timeline
kaniini commented 2019-06-01 05:18:20 +00:00
Member
Copy link

imo instead of excluding statuses, they should just not be included in the timeline list twice when the pins are fetched, so it should work this way:

Post 3 (pinned)
Post 999
[...]
Post 3
Post 2
Post 1

specifically, the problem is that if you scroll through a timeline after pinning a post, say, a week ago, as soon as you hit that post in the timeline (since it's added twice -- once for the pinning and once normally), scrolling past it brings up things in the past. so that means you can have a huge hole in the browsed timeline.

imo instead of excluding statuses, they should just not be included in the timeline list twice when the pins are fetched, so it should work this way: ``` Post 3 (pinned) Post 999 [...] Post 3 Post 2 Post 1 ``` specifically, the problem is that if you scroll through a timeline after pinning a post, say, a week ago, as soon as you hit that post in the timeline (since it's added twice -- once for the pinning and once normally), scrolling past it brings up things in the past. so that means you can have a huge hole in the browsed timeline.
hj commented 2019-06-01 09:19:16 +00:00
Member
Copy link

that's what i'm suggesting

that's what i'm suggesting
tae commented 2019-07-20 20:58:09 +00:00
Author
Member
Copy link

updated 😄

updated :smile:
tae commented 2019-07-24 16:37:38 +00:00
Author
Member
Copy link

bump @hj

bump @hj
hj commented 2019-07-24 18:04:30 +00:00
Member
Copy link

it took me like 30 minutes to understand why it's working and I think it could be simplified to
[REDACTED]

~~it took me like 30 minutes to understand why it's working and I think it could be simplified to~~ [REDACTED]
hj commented 2019-07-24 18:04:49 +00:00
Member
Copy link

probably should say Statuses tho

probably should say Statuses tho
hj commented 2019-07-24 18:05:12 +00:00
Member
Copy link

probably needs unit tests either way

probably needs unit tests either way
hj commented 2019-07-24 18:05:58 +00:00
Member
Copy link

ok, hold that thought, wait, i forgot that there might be multiple pinned posts

ok, hold that thought, wait, i forgot that there might be multiple pinned posts
hj commented 2019-07-24 18:07:50 +00:00
Member
Copy link

ok,
first things first - as I said, unit tests are a must.
secondly - use !.includes() instead of indexOf() === -1

ok, first things first - as I said, unit tests are a must. secondly - use `!.includes()` instead of `indexOf() === -1`
hj commented 2019-07-24 18:08:27 +00:00
Member
Copy link

works well but needs unit tests for exclusion logic

works well but needs unit tests for exclusion logic
tae commented 2019-07-25 18:19:13 +00:00
Author
Member
Copy link

fixed @hj

fixed @hj
hj commented 2019-07-25 20:52:13 +00:00
Member
Copy link

this test is identical to the previous one

this test is identical to the previous one
hj commented 2019-07-25 20:55:05 +00:00
Member
Copy link

i think you can use chai's .to.have.ordered.members() instead of verifying that "difference" is empty

i think you can use chai's `.to.have.ordered.members()` instead of verifying that "difference" is empty
hj commented 2019-07-25 20:56:20 +00:00
Member
Copy link

i.e.

      expect(getExcludedStatusIdsByPinning(statuses, pinnedStatusIds)).to.have.ordered.members(statusIds)
i.e. ```suggestion expect(getExcludedStatusIdsByPinning(statuses, pinnedStatusIds)).to.have.ordered.members(statusIds) ```
hj commented 2019-07-25 20:57:59 +00:00
Member
Copy link

or is it?

or is it?
hj commented 2019-07-25 20:58:36 +00:00
Member
Copy link

or use whatever you need because difference makes it much harder to read than it should be.

or use whatever you need because difference makes it much harder to read than it should be.
tae commented 2019-07-26 03:00:08 +00:00
Author
Member
Copy link

no, it isn't.

no, it isn't.
tae commented 2019-07-26 03:36:28 +00:00
Author
Member
Copy link

updated.

updated.
hj commented 2019-07-26 07:42:53 +00:00
Member
Copy link
  1. do not use constants in expect() part, you should put part that changes in it.
  2. .to.include.members() will work as to check subset, i.e.
expect([1,2,3]).to.include.members([1,2]) // CORRECT
expect([1,2,3]).to.include.members([1]) // CORRECT
expect([1,2,3]).to.have.members([1,2]) // ERROR
expect([1,2,3]).to.have.members([1,3,2]) // CORRECT
expect([1,2,3]).to.have.ordered.members([1,3,2]) // ERROR
expect([1,2,3]).to.have.ordered.members([1,2,3]) // CORRECT
  1. the 'should not return any status ids not listed in the given statuses' is a bit confusing, at least to me.
  2. both this and previous case testing that getExcludedStatusIdsByPinning(...) contain [1,3,5] or [1,2,3,4] or rather other way around, making it twice as confusing

i'm just gonna say it here:

Unit tests MUST BE clear to read and strict.

You should be able to understand what the input values are, what output values are EXPECTED to be, what side-effects are etc. Comments are welcome too.

1. do not use constants in `expect()` part, you should put part that changes in it. 2. `.to.include.members()` will work as to check subset, i.e. ```js expect([1,2,3]).to.include.members([1,2]) // CORRECT expect([1,2,3]).to.include.members([1]) // CORRECT expect([1,2,3]).to.have.members([1,2]) // ERROR expect([1,2,3]).to.have.members([1,3,2]) // CORRECT expect([1,2,3]).to.have.ordered.members([1,3,2]) // ERROR expect([1,2,3]).to.have.ordered.members([1,2,3]) // CORRECT ``` 3. the `'should not return any status ids not listed in the given statuses'` is a bit confusing, at least to me. 4. both this and previous case testing that `getExcludedStatusIdsByPinning(...)` contain `[1,3,5]` or `[1,2,3,4]` or rather other way around, making it twice as confusing i'm just gonna say it here: ## Unit tests **MUST BE** clear to read and strict. You should be able to understand what the input values are, what output values are EXPECTED to be, what side-effects are etc. Comments are welcome too.
hj commented 2019-07-26 07:53:07 +00:00
Member
Copy link

From the name I assume test is checking that output should never have anything that is not in pinnedStatusIds
So, by this logic, given the input data it should only output any of [1,3,5] but not not-pinned 2, 4
So first we must clarify that unpinned posts are and check that output does not include them:

      const pinnedStatusIds = [1, 3, 5]
      const unpinnedStatusIds = [2, 4] // cannot spread array since we want to preserve the order
      const result = getExcludedStatusIdsByPinning(statuses, pinnedStatusIds)
      expect(result).to.not.include.members(unpinnedStatusIds)
From the name I assume test is checking that output should never have anything that is not in `pinnedStatusIds` So, by this logic, given the input data it should only output any of `[1,3,5]` but not not-pinned `2, 4` So first we must clarify that unpinned posts are and check that output does not include them: ```suggestion:-1 const pinnedStatusIds = [1, 3, 5] const unpinnedStatusIds = [2, 4] // cannot spread array since we want to preserve the order const result = getExcludedStatusIdsByPinning(statuses, pinnedStatusIds) expect(result).to.not.include.members(unpinnedStatusIds) ```
hj commented 2019-07-26 07:59:59 +00:00
Member
Copy link

From the name I assume test is checking that output should never have anything that is not in statuses...

which is dumb and pointless, even more so than previous test, i honestly see little value in ever checking this, not to mention checking it would involve check each and every item in array to be one of statusIds which would be something like

const result = getExcludedStatusIdsByPinning(statuses, pinnedStatusIds);
result.forEach(item => expect(item).to.be.oneOf(statusIds));

which is increased complexity, otherwise it would be like

const result = getExcludedStatusIdsByPinning(statuses, pinnedStatusIds);
expect(result).to.have.length(666) // or whatever
expect(result[0]).to.be.oneOf(statusIds)
expect(result[1]).to.be.oneOf(statusIds)
// ... repeat as needed
From the name I assume test is checking that output should never have anything that is not in `statuses`... which is dumb and pointless, even more so than previous test, i honestly see little value in ever checking this, not to mention checking it would involve check each and every item in array to be one of `statusIds` which would be something like ```js const result = getExcludedStatusIdsByPinning(statuses, pinnedStatusIds); result.forEach(item => expect(item).to.be.oneOf(statusIds)); ``` which is increased complexity, otherwise it would be like ```js const result = getExcludedStatusIdsByPinning(statuses, pinnedStatusIds); expect(result).to.have.length(666) // or whatever expect(result[0]).to.be.oneOf(statusIds) expect(result[1]).to.be.oneOf(statusIds) // ... repeat as needed ```
hj commented 2019-07-26 08:02:13 +00:00
Member
Copy link

This test is good, there should be more like it.
image

This test is good, there should be more like it. ![image](/attachments/0ae42676-4861-4bfe-836d-46be1c29a2c1)
image.png
800 KiB
tae commented 2019-07-28 20:55:55 +00:00
Author
Member
Copy link

updated

updated
hj commented 2019-07-28 21:05:21 +00:00
Member
Copy link

ok, fine, i'll write some more good unit tests myself

ok, fine, i'll write some more good unit tests myself

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
BE Request

   
Backlog

   
Better behavior

   
Blocked by deps

   
Bug

   
Code Review

   
Discussion

   
Doing

   
Feature Request / Enhancement

   
In the shining bright future maybe

   
It's complicated

   
MS Edge

   
Mememoon (or other niche browsers)

   
Missing API

   
Missing feature

   
Missing l10n/i18n

   
NL1
Instance Admin Dashboard

  
NL2
Improved Mobile Experience

  
NL4
Improved instance customization and QoL options

  
Need to verify on develop

   
RELEASE BLOCKER

   
Reassign or Close

   
Refactor

   
Regression

   
Safari

   
To Do

   
User story

   
Waiting on godot

   
accessibility
https://git.pleroma.social/pleroma/pleroma-fe/-/snippets/7746

  
better documentation

   
chore

   
confirmed

   
easy ticket

   
incident
Denotes a disruption to IT services and the associated issues require immediate attention

  
l10n update

   
mastoapi

   
mobile

   
needs design

   
needs-info

   
needs-review

   
stupid
No labels
BE Request
Backlog
Better behavior
Blocked by deps
Bug
Code Review
Discussion
Doing
Feature Request / Enhancement
In the shining bright future maybe
It's complicated
MS Edge
Mememoon (or other niche browsers)
Missing API
Missing feature
Missing l10n/i18n
NL1
NL2
NL4
Need to verify on develop
RELEASE BLOCKER
Reassign or Close
Refactor
Regression
Safari
To Do
User story
Waiting on godot
accessibility
better documentation
chore
confirmed
easy ticket
incident
l10n update
mastoapi
mobile
needs design
needs-info
needs-review
stupid
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
pleroma/pleroma-fe!2081
No description provided.