pleroma/pleroma-fe
2
1
Fork 2
Code Issues 345 Pull requests 30 Projects Releases Packages Wiki Activity

Allow remove of banner, avatar images issue #353 v2 #2423

Closed
shpuld wants to merge 33 commits from gitlab-mr-iid-1156 into develop
pull from: gitlab-mr-iid-1156
merge into: pleroma:develop
Conversation 47 Commits 33 Files changed 10 +153 -76
shpuld commented 2020-06-27 10:21:57 +00:00
Owner
Copy link

Branched off the original MR at #2146 to fix conflicts, pushing to remote too much effort.

Branched off the original MR at https://git.pleroma.social/pleroma/pleroma-fe/pulls/2146 to fix conflicts, pushing to remote too much effort.
lambadalambda commented 2020-06-27 10:23:51 +00:00
Owner
Copy link

Backend returns what?

Backend returns what?
shpuld commented 2020-06-27 10:33:08 +00:00
Author
Owner
Copy link

no clue, I'm still investigating and testing stuff from this branch

no clue, I'm still investigating and testing stuff from this branch
shpuld commented 2020-06-27 13:38:35 +00:00
Author
Owner
Copy link

image

this is what the request returns

![image](/attachments/35a6ed61-b401-427b-a201-9540ad02e629) this is what the request returns
image.png
91 KiB
shpuld commented 2020-06-27 15:00:28 +00:00
Author
Owner
Copy link

image

Changed the avatar reset button style to overlay the avatar, didn't do this for banner/background which don't have a different length button next to them, banner is also variable sized and background doesn't even have a preview.

![image](/attachments/30a79501-1dfc-458c-a742-85e1f27ccc80) Changed the avatar reset button style to overlay the avatar, didn't do this for banner/background which don't have a different length button next to them, banner is also variable sized and background doesn't even have a preview.
image.png
58 KiB
shpuld commented 2020-06-27 15:01:17 +00:00
Author
Owner
Copy link

yo git wtf

yo git wtf
lambadalambda commented 2020-06-27 16:34:41 +00:00
Owner
Copy link

what the heck

what the heck
lambadalambda commented 2020-06-29 10:45:47 +00:00
Owner
Copy link

Which endpoint is being called here?

Which endpoint is being called here?
shpuld commented 2020-06-29 10:50:58 +00:00
Author
Owner
Copy link
const AVATAR_PATCH_URL = '/api/v1/pleroma/accounts/update_avatar'
const BANNER_PATCH_URL = '/api/v1/pleroma/accounts/update_banner'
const BACKGROUND_PATCH_URL = '/api/v1/pleroma/accounts/update_background'
```javascript const AVATAR_PATCH_URL = '/api/v1/pleroma/accounts/update_avatar' const BANNER_PATCH_URL = '/api/v1/pleroma/accounts/update_banner' const BACKGROUND_PATCH_URL = '/api/v1/pleroma/accounts/update_background' ```
shpuld commented 2020-06-29 10:51:31 +00:00
Author
Owner
Copy link

still possible that the code in this MR is using it wrong and mangling the result, but I haven't yet investigated that far

still possible that the code in this MR is using it wrong and mangling the result, but I haven't yet investigated that far
lambadalambda commented 2020-06-29 10:53:15 +00:00
Owner
Copy link

I like the 'x' to remove it, but it does feel a bit strange that it's only here and not with all of them. Not a dealbreaker but i think having more or less identical 'reset' or 'clear' buttons is for all of them is better

I like the 'x' to remove it, but it does feel a bit strange that it's only here and not with all of them. Not a dealbreaker but i think having more or less identical 'reset' or 'clear' buttons is for all of them is better
lambadalambda commented 2020-06-29 10:54:19 +00:00
Owner
Copy link

I think all of these were added because of old GS compatibility reasons. You can actually remove all of these by just sending a null as an argument to the normal update_credentials endpoint.

I think all of these were added because of old GS compatibility reasons. You can actually remove all of these by just sending a `null` as an argument to the normal `update_credentials` endpoint.
shpuld commented 2020-06-29 13:04:41 +00:00
Author
Owner
Copy link

Ill try that

Ill try that
shpuld commented 2020-06-29 13:05:54 +00:00
Author
Owner
Copy link

if I want X for all, I'll need to change the styles a bit I think to make them match otherwise too, right now avatar setting looks quite different

if I want X for all, I'll need to change the styles a bit I think to make them match otherwise too, right now avatar setting looks quite different
shpuld commented 2020-06-29 13:07:34 +00:00
Author
Owner
Copy link

Lies, invalid request

Lies, invalid request
lambadalambda commented 2020-06-29 13:38:04 +00:00
Owner
Copy link

i tested it just now, what are you testing against?

i tested it just now, what are you testing against?
shpuld commented 2020-06-29 14:37:23 +00:00
Author
Owner
Copy link

dontbulling.me

dontbulling.me
shpuld commented 2020-06-29 15:21:22 +00:00
Author
Owner
Copy link

which method should it use? updateCredentials is using PATCH for actually setting a new one. I tried with both avatar and banner, both return 403 with message "Invalid request". I don't see anything obviously wrong in the request

image

which method should it use? updateCredentials is using PATCH for actually setting a new one. I tried with both avatar and banner, both return 403 with message "Invalid request". I don't see anything obviously wrong in the request ![image](/attachments/14c9a623-4c51-4ae0-b98f-40b6dc21105a)
image.png
78 KiB
lambadalambda commented 2020-06-29 15:35:08 +00:00
Owner
Copy link

same as for setting a new one. just a hunch, but can you try sending the payload as json instead of form?

same as for setting a new one. just a hunch, but can you try sending the payload as json instead of form?
shpuld commented 2020-06-29 16:12:51 +00:00
Author
Owner
Copy link

I think the masto docs explicitly say to use forms

I think the masto docs explicitly say to use forms
lambadalambda commented 2020-06-29 17:24:19 +00:00
Owner
Copy link

try json. Source: dude trust me

try json. Source: dude trust me
shpuld commented 2020-06-29 18:24:43 +00:00
Author
Owner
Copy link

because going against the spec is always the way to go, fix the BE parts anyway no matter how we do it (t. not booting up the dev pc to change the code right now

because going against the spec is always the way to go, fix the BE parts anyway no matter how we do it (t. not booting up the dev pc to change the code right now
lambadalambda commented 2020-06-30 08:44:38 +00:00
Owner
Copy link

don't worry, i'll check what the actual issue is :)

don't worry, i'll check what the actual issue is :)
lambadalambda commented 2020-06-30 08:45:35 +00:00
Owner
Copy link

also maybe a stupid question, but did you send the authorization header?

also maybe a stupid question, but did you send the authorization header?
lambadalambda commented 2020-06-30 08:47:48 +00:00
Owner
Copy link

(sidenote: both form data and json should work and be equivalent, see https://docs.joinmastodon.org/client/intro/)

(sidenote: both form data and json should work and be equivalent, see https://docs.joinmastodon.org/client/intro/)
shpuld commented 2020-06-30 16:44:19 +00:00
Author
Owner
Copy link

yeah I'm pretty sure I checked that the header was there, I made it use the same function calls that successfully set the avatar too. those wouldn't work without auth right?

yeah I'm pretty sure I checked that the header was there, I made it use the same function calls that successfully set the avatar too. those wouldn't work without auth right?
shpuld commented 2020-06-30 16:45:28 +00:00
Author
Owner
Copy link

I can try it with json too I guess when I get back to it tomorrow

I can try it with json too I guess when I get back to it tomorrow
lambadalambda commented 2020-06-30 20:02:39 +00:00
Owner
Copy link

it shouldn't respond 403 if the auth is missing, i'll take a look at the backend again tomorrow. I did write tests for these exact cases yesterday, but as knuth said, i only proved it to be correct and didn't run it.

it shouldn't respond 403 if the auth is missing, i'll take a look at the backend again tomorrow. I did write tests for these exact cases yesterday, but as knuth said, i only proved it to be correct and didn't run it.
shpuld commented 2020-06-30 20:12:57 +00:00
Author
Owner
Copy link

if it wasn't providing the auth then there's something very strange going on with the FE code and should probably refactor things w

if it wasn't providing the auth then there's something very strange going on with the FE code and should probably refactor things w
shpuld commented 2020-06-30 20:43:53 +00:00
Author
Owner
Copy link

double checked, authorization header is being given when I get the 403

double checked, authorization header is being given when I get the 403
shpuld commented 2020-06-30 20:48:53 +00:00
Author
Owner
Copy link

tried the json trick, it sure returned 200 with params 'header': null, but the banner wasn't reset, not even after reloading...

tried the json trick, it sure returned 200 with params `'header': null`, but the banner wasn't reset, not even after reloading...
lambadalambda commented 2020-06-30 20:57:48 +00:00
Owner
Copy link

@_@

i'll check myself tomorrow, thanks for trying!

@_@ i'll check myself tomorrow, thanks for trying!
shpuld commented 2020-07-01 08:11:36 +00:00
Author
Owner
Copy link

not sure if it matters, but read the masto docs more closely, it says whenever uploading files you should use multipart/form-data, now the real question is, if you send null where it should be a file, is it ok to use json 🤔

not sure if it matters, but read the masto docs more closely, it says whenever uploading files you should use multipart/form-data, now the real question is, if you send null where it should be a file, is it ok to use json :thinking:
shpuld commented 2020-07-01 08:28:07 +00:00
Author
Owner
Copy link

adjusted things:

  • added background preview
  • scaled down the previews of banner and background
  • made the reset button the transparent X on all 3
  • removed the "your current avatar" stuff which was as useful as labels in political cartoons

image

adjusted things: - added background preview - scaled down the previews of banner and background - made the reset button the transparent X on all 3 - removed the "your current avatar" stuff which was as useful as labels in political cartoons ![image](/attachments/9927e66e-c326-49bc-b5bf-132c958cdf02)
image.png
318 KiB
shpuld commented 2020-07-01 08:30:21 +00:00
Author
Owner
Copy link

DOES NOT WORK until the null-setting for update_credentials is figured out and fixed

DOES NOT WORK until the null-setting for update_credentials is figured out and fixed
lambadalambda commented 2020-07-01 08:39:09 +00:00
Owner
Copy link

checked it now, indeed my test to check that this works was wrong, going into it again...

checked it now, indeed my test to check that this works was wrong, going into it again...
lambadalambda commented 2020-07-01 08:51:26 +00:00
Owner
Copy link

So here's what I found:

  • Resetting the images in the update_credentials call does not work, we filter nil-values out in multiple places
  • The test I wrote to confirm that this works was wrong, so I was under the wrong impression that it worked
  • It doesn't work in mastodon either, it returns a 'bad request'
  • Please use the specific reset endpoints again like you did originally
  • Sorry for the back-and-forth ;_;
So here's what I found: - Resetting the images in the update_credentials call does not work, we filter nil-values out in multiple places - The test I wrote to confirm that this works was wrong, so I was under the wrong impression that it worked - It doesn't work in mastodon either, it returns a 'bad request' - Please use the specific reset endpoints again like you did originally - Sorry for the back-and-forth ;_;
shpuld commented 2020-07-01 08:52:06 +00:00
Author
Owner
Copy link

🔫

:gun:
shpuld commented 2020-07-01 08:52:48 +00:00
Author
Owner
Copy link

how does mastodon reset avatar/header or does it do it at all?

how does mastodon reset avatar/header or does it do it at all?
lambadalambda commented 2020-07-01 08:59:03 +00:00
Owner
Copy link

They do it in the non-api parts of mastodon, in the backend-generated html settings page

They do it in the non-api parts of mastodon, in the backend-generated html settings page
hj commented 2020-07-01 09:01:09 +00:00
Owner
Copy link

POST https://mastodon.social/settings/profile/pictures/header

form data:
"_method":"delete",
"authenticity_token":"tenshieatingacorndog"

and it's a page navigation, not an XHR

POST https://mastodon.social/settings/profile/pictures/header form data: "_method":"delete", "authenticity_token":"tenshieatingacorndog" and it's a page navigation, not an XHR
hj commented 2020-07-04 22:27:15 +00:00
Owner
Copy link
        this.submitBackground(null)

otherwise it errors out

```suggestion:-0+0 this.submitBackground(null) ``` otherwise it errors out
hj commented 2020-07-04 22:30:01 +00:00
Owner
Copy link

code-wise looks ok, but currently doesn't work.

code-wise looks ok, but currently doesn't work.
shpuld commented 2020-07-05 06:43:28 +00:00
Author
Owner
Copy link

https://git.pleroma.social/pleroma/pleroma/-/issues/1918

https://git.pleroma.social/pleroma/pleroma/-/issues/1918
shpuld commented 2020-07-05 06:46:59 +00:00
Author
Owner
Copy link

discussed it elsewhere and thought it's probably not a good idea to start using and locking in the ugly custom endpoints for this, but it'd be better to have a less broken way to do things. issue about it here https://git.pleroma.social/pleroma/pleroma/-/issues/1918

discussed it elsewhere and thought it's probably not a good idea to start using and locking in the ugly custom endpoints for this, but it'd be better to have a less broken way to do things. issue about it here https://git.pleroma.social/pleroma/pleroma/-/issues/1918
shpuld commented 2020-07-07 15:44:59 +00:00
Author
Owner
Copy link

ok, I have changed the code to work with pleroma/pleroma#6107 when it gets merged

ok, I have changed the code to work with https://git.pleroma.social/pleroma/pleroma/pulls/6107 when it gets merged
shpuld commented 2020-07-08 09:46:36 +00:00
Author
Owner
Copy link

this now works against latest develop

this now works against latest develop
lambadalambda commented 2020-07-08 10:11:10 +00:00
Owner
Copy link

🎉

:tada:

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
3 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!2423
No description provided.