Plume/Plume
9
18
Fork 24
Code Issues 180 Pull requests 8 Releases 8 Wiki Activity

Add a fqn field to blogs and users #457

Merged
elegaanz merged 6 commits from fqn into master 2019-03-06 17:28:10 +00:00
Conversation 3 Commits 6 Files changed 39 +258 -141
elegaanz commented 2019-03-05 10:16:55 +00:00 (Migrated from github.com)
Copy link

Fixes #319

Fixes #319
codecov[bot] commented 2019-03-05 12:29:19 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #457 into master will increase coverage by 0.75%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #457      +/-   ##
==========================================
+ Coverage   26.49%   27.25%   +0.75%     
==========================================
  Files          64       64              
  Lines        7111     7310     +199     
==========================================
+ Hits         1884     1992     +108     
- Misses       5227     5318      +91
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/457?src=pr&el=h1) Report > Merging [#457](https://codecov.io/gh/Plume-org/Plume/pull/457?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/2a188abfa152efe7a2794174279e3b9da4a809d1?src=pr&el=desc) will **increase** coverage by `0.75%`. > The diff coverage is `50%`. ```diff @@ Coverage Diff @@ ## master #457 +/- ## ========================================== + Coverage 26.49% 27.25% +0.75% ========================================== Files 64 64 Lines 7111 7310 +199 ========================================== + Hits 1884 1992 +108 - Misses 5227 5318 +91 ```
igalic (Migrated from github.com) reviewed 2019-03-05 17:03:15 +00:00
igalic (Migrated from github.com) left a comment
Copy link

❓

❓
migrations/sqlite/2019-03-05-082846_add_fqn/up.sql
igalic (Migrated from github.com) commented 2019-03-05 16:48:28 +00:00
Copy link

what makes you think i managed to correctly create my instance in go?

what makes you think i managed to correctly create my instance in go?
trinity-1686a commented 2019-03-05 19:05:05 +00:00
Owner
Copy link

I'm wondering. On a brand new, uninitialized instance, does it works? The select would return 0 rows, so does the CASE never match, or does it fail?

I'm wondering. On a brand new, uninitialized instance, does it works? The select would return 0 rows, so does the CASE never match, or does it fail?
elegaanz commented 2019-03-05 19:07:57 +00:00 (Migrated from github.com)
Copy link

I think it will never run, since there are no blogs or users, so no problem.

I think it will never run, since there are no blogs or users, so no problem.
trinity-1686a reviewed 2019-03-05 19:09:47 +00:00
migrations/sqlite/2019-03-05-082846_add_fqn/up.sql
@ -0,0 +4,4 @@
(CASE WHEN (SELECT local FROM instances WHERE id = instance_id) THEN
actor_id
ELSE
(actor_id || '@' || (SELECT public_domain FROM instances WHERE id = instance_id LIMIT 1))
trinity-1686a commented 2019-03-05 19:09:47 +00:00
Owner
Copy link

why LIMIT 1? Isn't id unique?

why LIMIT 1? Isn't id unique?
elegaanz (Migrated from github.com) reviewed 2019-03-05 19:47:35 +00:00
migrations/sqlite/2019-03-05-082846_add_fqn/up.sql
@ -0,0 +4,4 @@
(CASE WHEN (SELECT local FROM instances WHERE id = instance_id) THEN
actor_id
ELSE
(actor_id || '@' || (SELECT public_domain FROM instances WHERE id = instance_id LIMIT 1))
elegaanz (Migrated from github.com) commented 2019-03-05 19:47:35 +00:00
Copy link

Just to be sure. 😛

Just to be sure. :stuck_out_tongue:
trinity-1686a reviewed 2019-03-06 09:59:16 +00:00
migrations/sqlite/2019-03-05-082846_add_fqn/down.sql
trinity-1686a commented 2019-03-06 09:59:16 +00:00
Owner
Copy link

sqlite doesn't allow to drop column, you have to do the annoying copy thing

sqlite doesn't allow to drop column, you have to do the annoying copy thing
elegaanz (Migrated from github.com) reviewed 2019-03-06 10:04:04 +00:00
migrations/sqlite/2019-03-05-082846_add_fqn/down.sql
elegaanz (Migrated from github.com) commented 2019-03-06 10:04:04 +00:00
Copy link

Ah, yes, forgot about that… 😑

Ah, yes, forgot about that… :expressionless:
trinity-1686a reviewed 2019-03-06 10:54:07 +00:00
trinity-1686a left a comment
Owner
Copy link

the no_fqn tables are maybe to close of their "with fqn" counterpart?

the no_fqn tables are maybe to close of their "with fqn" counterpart?
migrations/sqlite/2019-03-05-082846_add_fqn/down.sql
trinity-1686a commented 2019-03-06 10:53:17 +00:00
Owner
Copy link

blogs_no_fqn, fqn TEXT NOT NULL...

`blogs_no_fqn`, `fqn TEXT NOT NULL`...
trinity-1686a commented 2019-03-06 10:53:36 +00:00
Owner
Copy link

users_no_fqn, fqn TEXT NOT NULL...

`users_no_fqn`, `fqn TEXT NOT NULL`...
elegaanz (Migrated from github.com) reviewed 2019-03-06 10:56:36 +00:00
migrations/sqlite/2019-03-05-082846_add_fqn/down.sql
elegaanz (Migrated from github.com) commented 2019-03-06 10:56:36 +00:00
Copy link

Ah, that's why it was telling me I was not supplying enough values when doing INSERT INTO blogs_no_fqn SELECT all, the, columns, but, not, fqn FROM blogs;. 😬

Ah, that's why it was telling me I was not supplying enough values when doing `INSERT INTO blogs_no_fqn SELECT all, the, columns, but, not, fqn FROM blogs;`. :grimacing:
trinity-1686a reviewed 2019-03-06 13:14:19 +00:00
plume-models/src/schema.rs
@ -43,6 +43,7 @@ table! {
ap_url -> Text,
private_key -> Nullable<Text>,
public_key -> Text,
fqn -> Text,
trinity-1686a commented 2019-03-06 13:14:19 +00:00
Owner
Copy link

this is not supposed to be Nullable, but I don't get where it come from, your migrations aren't touching this table

this is not supposed to be Nullable, but I don't get where it come from, your migrations aren't touching this table
elegaanz (Migrated from github.com) reviewed 2019-03-06 13:24:57 +00:00
plume-models/src/schema.rs
@ -43,6 +43,7 @@ table! {
ap_url -> Text,
private_key -> Nullable<Text>,
public_key -> Text,
fqn -> Text,
elegaanz (Migrated from github.com) commented 2019-03-06 13:24:57 +00:00
Copy link

Diesel is regenerating the schema when running migrations, so it changed when I tested with SQlite.

Diesel is regenerating the schema when running migrations, so it changed when I tested with SQlite.
trinity-1686a reviewed 2019-03-06 13:42:30 +00:00
plume-models/src/schema.rs
@ -43,6 +43,7 @@ table! {
ap_url -> Text,
private_key -> Nullable<Text>,
public_key -> Text,
fqn -> Text,
trinity-1686a commented 2019-03-06 13:42:30 +00:00
Owner
Copy link

For some reason, comment_seers's id is an INTEGER PRIMARY KEY AUTOINCREMENT, not an INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT on sqlite (but fine on pg I assume), that's why the scheme was invalid
Edit : I know why, this was introduced in this commit : fdfeeed6d9 , my bad

For some reason, comment_seers's id is an `INTEGER PRIMARY KEY AUTOINCREMENT`, not an `INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT` on sqlite (but fine on pg I assume), that's why the scheme was invalid Edit : I know why, this was introduced in this commit : fdfeeed6d9fa9dec92862366a8b646eb27d17422 , my bad
trinity-1686a approved these changes 2019-03-06 16:51:52 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
trinity-1686a
Labels
Clear labels   
A: API

Related to the REST API

  
A: Backend

Code running on the server

  
A: Federation

Stuff related to Federation

  
A: Front-End

Related to the front-end

  
A: I18N

Translations, and related code

  
A: Meta

More about project management or code than the project itself

  
A: Security

   
Build

The building, or installation process of Plume

  
C: Bug

Something isn't working

  
C: Discussion

We need to talk

  
C: Enhancement

New feature or request

  
C: Feature

This is a new feature

  
Compatibility

Compatibility with different browsers, readers and OS

  
Dependency

Related to an external package that Plume uses

  
Design

UI/UX related issues and PRs

  
Documentation

   
Good first issue

Good for newcomers

  
Help welcome

Extra attention is needed

  
Mobile

Issues affecting only mobile UX

  
Rendering

How elements're rendered out for the end user

  
S: Blocked

Something else needs to be fixed first

  
S: Duplicate

This issue or pull request already exists

  
S: Incomplete

This PR is not complete yet

  
S: Instance specific

Issues concern a limited number of instances

  
S: Invalid

This doesn't seem right

  
S: Needs Voting/Discussion

Need to be discussed by the community (on Loomio)

  
S: Ready for review

This PR is ready to be reviewed

  
Suggestion

Proposed ideas worth considering

  
S: Voted on Loomio

This is issue has been created after a vote on Loomio

  
S: Wontfix

This will not be worked on

No labels
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
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: Plume/Plume#457
No description provided.
Delete branch "fqn"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?