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

Add bio fields #6645

Merged
merged 6 commits into from Apr 14, 2018
Merged

Add bio fields #6645

merged 6 commits into from Apr 14, 2018

Conversation

@Gargron
Copy link
Member

@Gargron Gargron commented Mar 5, 2018

A feature that allows adding structured data to profiles. Based on @marrus-sh's implementation (could not find a glitch-soc PR for it though), except the fields are not part of the text.

Screenshots:

image

image

image


  • Display on public page
  • Return from REST API
  • Edit
  • Federate

How they are stored internally: As a JSONB column on the accounts table, which contains an array of { "name": "", "value": "" } objects. Why: Normally something like this would be implemented via a has_many association with separate database records for every field, but I do not want to add so many more eager loaded queries to everywhere, and the fields don't really need to be queried or updated standalone, so being nested in the account record seems okay.

How they are federated: Inside the actor's attachment tag they are represented as:

{
  "type": "PropertyValue",
  "name": "",
  "value": ""
}

Where PropertyValue and value come from the schema.org context, while name comes from the vanilla ActivityStreams context. The value can contain HTML, while the name cannot.

@Gargron Gargron force-pushed the feature-bio-fields branch 2 times, most recently from 50129f9 to 34c8658 Mar 6, 2018
@akihikodaki
Copy link
Collaborator

@akihikodaki akihikodaki commented Mar 8, 2018

I am not for this change.

A writer of the bio fields may be excited and appreciate that it allows markup like that, but I do not think such a markup will actually benefit readers. A colon-separated text or even a normal statements would work well.

@Gargron
Copy link
Member Author

@Gargron Gargron commented Mar 8, 2018

A writer of the bio fields may be excited and appreciate that it allows markup like that, but

No, this change does not plan for special markup, rather, edit profile form should have +/- field inputs.

@akihikodaki
Copy link
Collaborator

@akihikodaki akihikodaki commented Mar 8, 2018

No, this change does not plan for special markup, rather, edit profile form should have +/- field inputs.

I meant the table is the special markup.

@Gargron Gargron force-pushed the feature-bio-fields branch from 34c8658 to 97911e3 Apr 10, 2018
@Gargron
Copy link
Member Author

@Gargron Gargron commented Apr 11, 2018

@akihikodaki is normalizeAccount not run for the "me" account included with initialState in current master? i am trying to bring my "bio fields" PR up to date and running into this issue...

@ThisIsMissEm ThisIsMissEm mentioned this pull request Apr 11, 2018
1 of 1 task complete
@ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented Apr 12, 2018

I was thinking of doing a similar PR, but making the fields configurable by the admins (perhaps "default fields") and then also adding the ability for "locked" fields, which require an admin or certain application to edit.

Our use case is a default field that adds a "verified" status, and that being managed by an application, such that you can't manually add this field, and you could then know with certainty that we have actually verified the user.

This is important as we have users that may meet in person (potentially more likely than in other mastodon instances).

@akihikodaki
Copy link
Collaborator

@akihikodaki akihikodaki commented Apr 12, 2018

@Gargron

@akihikodaki is normalizeAccount not run for the "me" account included with initialState in current master? i am trying to bring my "bio fields" PR up to date and running into this issue...

I have just checked it and it looks fine for me. I added console.log:

diff --git a/app/javascript/mastodon/actions/importer/index.js b/app/javascript/mastodon/actions/importer/index.js
index 5b18cbc1d..907fe558d 100644
--- a/app/javascript/mastodon/actions/importer/index.js
+++ b/app/javascript/mastodon/actions/importer/index.js
@@ -45,6 +45,7 @@ export function importFetchedAccounts(accounts) {
   }
 
   accounts.forEach(processAccount);
+  console.log(normalAccounts);
   putAccounts(normalAccounts, !autoPlayGif);
 
   return importAccounts(normalAccounts);

And here is its output:

[{"id":"1","username":"admin","acct":"admin","display_name":"ねこ","locked":false,"created_at":"2018-04-05T12:02:21.752Z","note":"<p></p>","url":"http://localhost:3000/@admin","avatar":"http://localhost:3000/avatars/original/missing.png","avatar_static":"http://localhost:3000/avatars/original/missing.png","header":"http://localhost:3000/headers/original/missing.png","header_static":"http://localhost:3000/headers/original/missing.png","followers_count":1,"following_count":0,"statuses_count":9,"display_name_html":"ねこ","note_emojified":"<p></p>"}]
@Gargron Gargron force-pushed the feature-bio-fields branch from 97911e3 to edcac37 Apr 12, 2018
@Gargron
Copy link
Member Author

@Gargron Gargron commented Apr 12, 2018

@ThisIsMissEm I'm sorry but that won't work. A verified status like that should not federate, in my opinion, we should not teach users to trust anything like that that comes out of other servers. Besides, you don't even need something so complicated. Add a boolean column to the accounts table and put a checkmark on public profiles, somewhere where it can't be confused with any emoji.

@ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented Apr 12, 2018

@Gargron oh, hm. I hadn't really thought about that as a valid option — seemed like it'd cause a compatibility issue and be a bit too much of a fork. I'm still getting bearings as to managing the forked codebase and making sure we can still update to upstream.

@Gargron
Copy link
Member Author

@Gargron Gargron commented Apr 12, 2018

@ThisIsMissEm That's a really small change, I don't think you'll have problems with it.

Anyway, left todo in this PR: A UI on the "edit profile" page to edit these fields 😩

@ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented Apr 12, 2018

@Gargron for that, is it possible to use react components on the /admin or /settings pages? I'd be happy to do the UI for you if that's why you're leaving it as an open todo.

@Gargron Gargron force-pushed the feature-bio-fields branch 2 times, most recently Apr 13, 2018
spec/services/activitypub/process_account_service_spec.rb Outdated
account = subject.call('alice', 'example.com', payload)
expect(account.fields).to be_a Hash
expect(account.fields).to include('name' => 'Pronouns', 'value' => 'They/them')
expect(account.fields).to include('name' => 'Occupation', 'value' => 'Unit test')

This comment has been minimized.

@ThisIsMissEm

ThisIsMissEm Apr 13, 2018
Contributor

is it worth checking that account.fields doesn't have more than the payload declared?

@Gargron Gargron force-pushed the feature-bio-fields branch 2 times, most recently Apr 13, 2018
@Gargron Gargron force-pushed the feature-bio-fields branch to 0086549 Apr 13, 2018
Fix #121
@Cassolotl
Copy link

@Cassolotl Cassolotl commented Apr 13, 2018

These screenshots look awesome! :)

@Gargron Gargron merged commit 78ed4ab into master Apr 14, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Gargron Gargron deleted the feature-bio-fields branch Apr 14, 2018
@kevinmarks
Copy link

@kevinmarks kevinmarks commented Apr 14, 2018

Regarding verification, see the http://www.kevinmarks.com/distributed-verify.html approach. Add rel=me to these links, and check that they link back with a rel=me as well. Then you can tell that the same person can edit both pages.

@Gargron
Copy link
Member Author

@Gargron Gargron commented Apr 14, 2018

@kevinmarks I did add rel=me, it's the last commit.

@kevinmarks
Copy link

@kevinmarks kevinmarks commented Apr 14, 2018

@Wolf480pl
Copy link

@Wolf480pl Wolf480pl commented Apr 14, 2018

Does it render to semantic HTML, eg. using a microformat like hCard?

@Gargron
Copy link
Member Author

@Gargron Gargron commented Apr 14, 2018

It's a th/td table. Is there hCard markup for arbitrary information fields?

@Wolf480pl
Copy link

@Wolf480pl Wolf480pl commented Apr 14, 2018

It's a th/td table.

I'd probably go for dt/dd

Is there hCard markup for arbitrary information fields?

The whole idea of hCard is to tell the fields apart, eg. tag the email field as email, the nickname field as nickname, the birthday field as birthday, etc. Now that I think of it, it'd probably require some further UI to let the users optionally add hCard classes... sounds like a separate feature I guess?

@kevinmarks
Copy link

@kevinmarks kevinmarks commented Apr 14, 2018

@Gargron
Copy link
Member Author

@Gargron Gargron commented Apr 14, 2018

The whole idea of hCard is to tell the fields apart, eg. tag the email field as email, the nickname field as nickname, the birthday field as birthday, etc

Well, I never said this was an hCard. The whole idea of this function is to let users define whatever they deem necessary on their profile. So while it could be something common like "homepage", it could also be "github" or "pronouns" or "avatar made by" or "favourite cake".

@kevinmarks
Copy link

@kevinmarks kevinmarks commented Apr 14, 2018

@Gargron
Copy link
Member Author

@Gargron Gargron commented Apr 14, 2018

I actually don't see the benefit of it at all. Do any everyday non-technical users care about being vcard-comforming? Do they want that data to be machine-readable? In my impression, they do not.

S-H-GAMELINKS added a commit to S-H-GAMELINKS/mastodon that referenced this pull request Apr 15, 2018
S-H-GAMELINKS added a commit to S-H-GAMELINKS/mastodon that referenced this pull request Apr 16, 2018
S-H-GAMELINKS added a commit to S-H-GAMELINKS/mastodon that referenced this pull request Apr 16, 2018
@nightpool nightpool mentioned this pull request Apr 16, 2018
1 of 2 tasks complete
@sammy8806 sammy8806 mentioned this pull request Apr 25, 2018
0 of 2 tasks complete
Gargron added a commit that referenced this pull request May 21, 2018
* i18n: (zh-CN) #7532

* i18n: (zh-CN) #6984

* i18n: (zh-CN) #7391, #7507

* i18n: (zh-CN) #6998

* i18n: (zh-CN) #7074

* i18n: (zh-CN) #7000, #7032, #7131 (#7032, #7040)

* i18n: (zh-CN) #7130, #7188

* i18n: (zh-CN) #6486

* i18n: (zh-CN) #6292

* i18n: (zh-CN) #7347

* i18n: (zh-CN) #6661

* i18n: (zh-CN) #6425

* i18n: (zh-CN) #6597

* i18n: (zh-CN) #6695

* i18n: (zh-CN) #6325

* i18n: (zh-CN) #6460, #7375

* i18n: (zh-CN) #6872

* i18n: (zh-CN) #6818

* i18n: (zh-CN) #7452

* i18n: (zh-CN) #7176

* i18n: (zh-CN) #6460

* i18n: (zh-CN) #7213

* i18n: (zh-CN) #7376

* i18n: (zh-CN) #6556

* i18n: (zh-CN) #6645

* i18n: (zh-CN) #6448

* i18n: (zh-CN) #5303

* i18n: (zh-CN) #7445

* i18n: (zh-CN) Normalization and improvements

* i18n: (zh-CN) #7391

* i18n: (zh-CN) #6627

* i18n: (zh-CN) #6956, #7546

* i18n: (zh-CN) #6636

* i18n: (zh-CN) #6610, #6875

* i18n: (zh-CN) #6887

* i18n: (zh-CN) #4514

* i18n: (zh-CN) #6628

* i18n: (zh-CN) #6771

* i18n: (zh-CN) #6772

* i18n: (zh-CN) #7178

* i18n: (zh-CN) #7521

* i18n: (zh-CN) #6570

* i18n: (zh-CN) #6593

* i18n: (zh-CN) #6423

* i18n: (zh-CN) #6157

* i18n: (zh-CN) #7089

* i18n: (zh-CN) #6733

* i18n: (zh-CN) #7072

* i18n: (zh-CN) #6520

* i18n: (zh-CN) Improvment

* i18n: (zh-CN) #6631
aaronpk pushed a commit to indieweb/wiki that referenced this pull request Aug 19, 2018
@@ -75,6 +75,7 @@
t.boolean "memorial", default: false, null: false
t.bigint "moved_to_account_id"
t.string "featured_collection_url"
t.jsonb "fields"

This comment has been minimized.

@saper

saper Dec 27, 2018
Contributor

If fields is JSONB shouldn't account.fields be a Hash and not an Array?

See report on https://discourse.joinmastodon.org/t/error-on-profile-edit-for-single-user-nomethoderror-undefined-method-for-hash/1563

@nightpool
Copy link
Member

@nightpool nightpool commented Dec 27, 2018

@saper
Copy link
Contributor

@saper saper commented Dec 27, 2018

@saper no, because profile fields are sorted and json objects are arbitrary order

Thanks. But it seems to me that JSONB storage will lose that ordering? Is this the correct serialization (or am I completely confusing something)?

@nightpool
Copy link
Member

@nightpool nightpool commented Dec 27, 2018

@saper jsonb columns can be any json type—arrays, objects, or primitive values.

@saper
Copy link
Contributor

@saper saper commented Dec 27, 2018

Ok, but when ActiveRecord fetches JSONB from PostgreSQL, what do we get? I am just searching for clues related to https://discourse.joinmastodon.org/t/error-on-profile-edit-for-single-user-nomethoderror-undefined-method-for-hash/1563 - I think I am probably on a totally wrong path.

@nightpool
Copy link
Member

@nightpool nightpool commented Dec 27, 2018

@saper it will return an array for a json array, a hash for a json object, a corresponding rails value (string, number, boolean etc) for json primitive values.

@saper
Copy link
Contributor

@saper saper commented Dec 27, 2018

@saper it will return an array for a json array, a hash for a json object, a corresponding rails value (string, number, boolean etc) for json primitive values.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.