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 added the work in progress Not to be merged, currently being worked on label Mar 5, 2018
@Gargron Gargron force-pushed the feature-bio-fields branch 2 times, most recently from 50129f9 to 34c8658 Compare March 7, 2018 00:37
@akihikodaki
Copy link
Contributor

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 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
Contributor

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
Copy link
Member Author

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 task
@ThisIsMissEm
Copy link
Contributor

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
Contributor

@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
Copy link
Member Author

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

@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 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

@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 from 1bfe02e to 251fe66 Compare April 13, 2018 16:02
@Gargron Gargron removed the work in progress Not to be merged, currently being worked on label Apr 13, 2018
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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 from de4289e to 2a4f515 Compare April 13, 2018 17:52
@Cassolotl
Copy link

These screenshots look awesome! :)

@Gargron Gargron merged commit 78ed4ab into master Apr 14, 2018
@Gargron Gargron deleted the feature-bio-fields branch April 14, 2018 10:41
@kevinmarks
Copy link

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 commented Apr 14, 2018

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

@kevinmarks
Copy link

kevinmarks commented Apr 14, 2018 via email

@Wolf480pl
Copy link

Wolf480pl commented Apr 14, 2018

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

@Gargron
Copy link
Member Author

Gargron commented Apr 14, 2018

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

@Wolf480pl
Copy link

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 commented Apr 14, 2018 via email

@Gargron
Copy link
Member Author

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 commented Apr 14, 2018 via email

@Gargron
Copy link
Member Author

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
Gargron pushed 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"
Copy link
Contributor

@saper saper Dec 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 commented Dec 27, 2018 via email

@saper
Copy link
Contributor

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

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

@saper
Copy link
Contributor

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

@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 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants