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

Fix sent follow request detection #2177

Closed
rinpatch wants to merge 0 commits from gitlab-mr-iid-910 into develop
pull from: gitlab-mr-iid-910
merge into: pleroma:develop
Conversation 22 Commits - Files changed -
rinpatch commented 2019-08-09 11:24:33 +00:00
Member
Copy link

This fixes requestFollow using the relationship instead of user object,
resulting in sent always being false for locked users, and also removes
assumptions about follow request being sent, instead relying on
requested from user relationship.

This fixes `requestFollow` using the relationship instead of user object, resulting in `sent` always being false for locked users, and also removes assumptions about follow request being sent, instead relying on `requested` from user relationship.
hj commented 2019-08-09 11:30:10 +00:00
Member
Copy link

probably should stop retrying if we know that user is locked and we've sent the request?

although IIRC remote user can lock themselves and BE wouldn't know it just yet so it will still work like "request sent"

probably should stop retrying if we know that user is locked and we've sent the request? although IIRC remote user can lock themselves and BE wouldn't know it just yet so it will still work like "request sent"
hj commented 2019-08-09 11:31:20 +00:00
Member
Copy link

probably should be something like sent || following but i'm not sure how requested works actually

probably should be something like `sent || following` but i'm not sure how `requested` works actually
rinpatch commented 2019-08-09 11:32:34 +00:00
Author
Member
Copy link

requested becomes false when the follow is approved

`requested` becomes false when the follow is approved
rinpatch commented 2019-08-09 11:35:05 +00:00
Author
Member
Copy link

It won't start refetching the user if it is locked, see #2177/diffs

It won't start refetching the user if it is locked, see https://git.pleroma.social/pleroma/pleroma-fe/pulls/2177/diffs#20c41b74f87ed662c61e1ca4fa01fa3ba329b441_32_24
hj commented 2019-08-09 11:37:52 +00:00
Member
Copy link

it does right here, see line 12 in this very file - if we're not following user yet it will call function recursively until we run out of attempts.

it does right here, see line 12 in this very file - if we're not following user yet it will call function recursively until we run out of attempts.
hj commented 2019-08-09 11:39:38 +00:00
Member
Copy link

good opportunity to add a return after this line so that we exit the function and not refetch the user needlessly

good opportunity to add a `return` after this line so that we exit the function and not refetch the user needlessly
rinpatch commented 2019-08-09 11:44:41 +00:00
Author
Member
Copy link

like this?

like this?
hj commented 2019-08-09 11:46:45 +00:00
Member
Copy link

i mean the outcome of the function is - determine if we're following the user or at least we sent a follow request. Up until now we couldn't know if we sent a follow request instead of following, so we just assumed that if we are still not following user after several re-fetches (since BE does not update it right away sometimes) it means we sent a follow request. If we can know that we sent a follow request right away it would be incredibly helpful.

The thing is here, if i'm remembering correctly, that depending of outcome here FE will either display "Following!" (for true) or "Follow request sent!" (for false) so it should probably stay as return following here

i mean the outcome of the function is - determine if we're following the user or at least we sent a follow request. Up until now we couldn't know if we sent a follow request instead of following, so we just assumed that if we are still not following user after several re-fetches (since BE does not update it right away sometimes) it means we sent a follow request. If we can know that we sent a follow request right away it would be incredibly helpful. The thing is here, if i'm remembering correctly, that depending of outcome here FE will either display "Following!" (for true) or "Follow request sent!" (for false) so it should probably stay as `return following` here
hj commented 2019-08-09 11:51:25 +00:00
Member
Copy link

will be kinda broken for case when

  1. remote user becomes locked
  2. backend doesn't know about it yet
  3. we try to follow them, FE and BE believe that user is not locked
  4. it will keep retrying until we either run out of attempts or realize user is locked

what I'm thinking about is

if (!following && !sent && attempts <= 3)

will be kinda broken for case when 1. remote user becomes locked 2. backend doesn't know about it yet 3. we try to follow them, FE and BE believe that user is not locked 4. it will keep retrying until we either run out of attempts or realize user is locked what I'm thinking about is `if (!following && !sent && attempts <= 3)`
hj commented 2019-08-09 11:51:33 +00:00
Member
Copy link

not quite

not quite
hj commented 2019-08-09 11:52:19 +00:00
Member
Copy link

would probably be great to actually make it show "Request sent" in UI based on that field, since right now it will reset from navigating away from the page

would probably be great to actually make it show "Request sent" in UI based on that field, since right now it will reset from navigating away from the page
hj commented 2019-08-09 11:53:20 +00:00
Member
Copy link
          resolve({ sent })
```suggestion:-0+0 resolve({ sent }) ```
rinpatch commented 2019-08-09 12:00:55 +00:00
Author
Member
Copy link

The thing is here, if i'm remembering correctly, that depending of outcome here FE will either display "Following!" (for true) or "Follow request sent!" (for false)

No, it will show "Follow Request Sent" when sent: true and user.following: false

> The thing is here, if i'm remembering correctly, that depending of outcome here FE will either display "Following!" (for true) or "Follow request sent!" (for false) No, it will show "Follow Request Sent" when sent: true and user.following: false
rinpatch commented 2019-08-09 12:03:08 +00:00
Author
Member
Copy link

sent will be true for non-locked users as well, if the backend didn't yet recieve an approval from the remote server

`sent` will be true for non-locked users as well, if the backend didn't yet recieve an approval from the remote server
hj commented 2019-08-09 12:07:09 +00:00
Member
Copy link

now that's what i call useless

now that's what i call useless
hj commented 2019-08-09 12:08:48 +00:00
Member
Copy link

yeah it can remain as is then

i guess i expected too much

yeah it can remain as is then i guess i expected too much
hj commented 2019-08-09 12:21:20 +00:00
Member
Copy link

ok no, this still can be improved i believe, let me formalize it

ok no, this still can be improved i believe, let me formalize it
hj commented 2019-08-09 12:25:12 +00:00
Member
Copy link

since we have the info now it would make sense to only really confirm that we've sent the request when we actually know it

      if (updated.following || (user.locked && user.requested)) {
since we have the info now it would make sense to only really confirm that we've sent the request when we actually know it ```suggestion:-0+0 if (updated.following || (user.locked && user.requested)) { ```
hj commented 2019-08-09 12:26:16 +00:00
Member
Copy link
  if (!following && !(locked && sent) && attempt <= 3) {
```suggestion:-0+0 if (!following && !(locked && sent) && attempt <= 3) { ```
hj commented 2019-08-09 12:29:12 +00:00
Member
Copy link

LGTM, if you want, displaying the "request sent" could be done in a separate MR, just make an issue about it.

LGTM, if you want, displaying the "request sent" could be done in a separate MR, just make an issue about it.
rinpatch commented 2019-08-09 12:52:21 +00:00
Author
Member
Copy link

Opened #635

Opened #635
hj commented 2019-08-09 12:58:04 +00:00
Member
Copy link

if anything breaks i'm gonna fix it myself - that code needs more comments anyway

if anything breaks i'm gonna fix it myself - that code needs more comments anyway

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
2 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!2177
No description provided.