LBRYCommunity/spee.ch
1
0
Fork 0
Code Issues 95 Pull requests 2 Projects Releases 4 Packages Wiki Activity

Authentication #170

Merged
bones7242 merged 43 commits from authentication into master 2017-09-29 02:29:22 +02:00
Conversation 1 Commits 43 Files changed 59 +1776 -766
bones7242 commented 2017-09-21 21:12:14 +02:00 (Migrated from github.com)
Copy link

PR in progress (almost done, currently testing).

PR in progress (almost done, currently testing).
kauffj (Migrated from github.com) requested changes 2017-09-22 15:57:00 +02:00
kauffj (Migrated from github.com) left a comment
Copy link

Good progress.

Did not finish this review prior to standup, but likely plenty to work on just from this feedback.

Good progress. Did not finish this review prior to standup, but likely plenty to work on just from this feedback.
config/default.json
kauffj (Migrated from github.com) commented 2017-09-22 15:38:10 +02:00
Copy link

Great design here! Love that this makes it easy to use different channels on staging/dev and production as well as easy to change if forked.

Great design here! Love that this makes it easy to use different channels on staging/dev and production as well as easy to change if forked.
config/default.json
@ -9,3 +10,2 @@
"Database": "lbry",
"Username": "none",
"Password": "none"
"Username": null,
kauffj (Migrated from github.com) commented 2017-09-22 15:38:55 +02:00
Copy link

Is there a reason this file uses "none" as a string instead of null or something falsier?

Is there a reason this file uses `"none"` as a string instead of `null` or something falsier?
controllers/publishController.js
kauffj (Migrated from github.com) commented 2017-09-22 15:42:45 +02:00
Copy link

"This name is already in use by spee.ch"

"Claim" is an unusual word that is very LBRY specific. Let's avoid using it until we have to.

"This name is already in use by spee.ch" "Claim" is an unusual word that is very LBRY specific. Let's avoid using it until we have to.
kauffj (Migrated from github.com) commented 2017-09-22 15:43:30 +02:00
Copy link

Does the daemon support two publishes to the same name if made from different channels?

Does the daemon support two publishes to the same name if made from different channels?
controllers/publishController.js
@ -36,14 +38,32 @@ module.exports = {
fileType,
nsfw : publishParams.metadata.nsfw,
kauffj (Migrated from github.com) commented 2017-09-22 15:51:40 +02:00
Copy link

[file, claim] = result

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

`[file, claim] = result` https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment
helpers/handlebarsHelpers.js
@ -0,0 +8,4 @@
return new Handlebars.SafeString(
`<script>
(function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
(i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
kauffj (Migrated from github.com) commented 2017-09-22 15:52:34 +02:00
Copy link

Why do these have to be in a helper instead of just in a normal template?

(I know nothing about Handlebars -- you can just say it has to be this way if that's the case :) )

Why do these have to be in a helper instead of just in a normal template? (I know nothing about Handlebars -- you can just say it has to be this way if that's the case :) )
helpers/lbryApi.js
@ -123,0 +118,4 @@
},
})
.then(response => {
handleResponse(response, resolve, reject);
kauffj (Migrated from github.com) commented 2017-09-22 15:50:35 +02:00
Copy link

Should be a const somewhere

Should be a const somewhere
helpers/lbryApi.js
kauffj (Migrated from github.com) commented 2017-09-22 15:50:57 +02:00
Copy link

Should also probably be a const, assuming this is the same value for channels and publishes.

Should also probably be a const, assuming this is the same value for channels and publishes.
helpers/publishHelpers.js
@ -72,3 +70,3 @@
if (title === '' || title === null) {
if (title === null || title === '') {
title = name;
}
kauffj (Migrated from github.com) commented 2017-09-22 15:51:18 +02:00
Copy link

description.trim() === ''

`description.trim() === ''`
models/user.js
@ -0,0 +1,29 @@
module.exports = (sequelize, { STRING }) => {
const User = sequelize.define(
'User',
kauffj (Migrated from github.com) commented 2017-09-22 15:47:09 +02:00
Copy link

Is this a "User" or a "Channel"? I understand these have a 1-1 correspondence right now, but these are likely not the same thing as spee.ch continues to grow/evolve.

I think it is likely that this should be two tables, a User table and a Channel table, with Channel having a foreign key to User.

In addition to being more semantic, this would facilitate growth into a design where one user can access multiple channels from a single session/login without a tricky later refactor.

Is this a "User" or a "Channel"? I understand these have a 1-1 correspondence right now, but these are likely not the same thing as spee.ch continues to grow/evolve. I think it is likely that this should be two tables, a User table and a Channel table, with Channel having a foreign key to User. In addition to being more semantic, this would facilitate growth into a design where one user can access multiple channels from a single session/login without a tricky later refactor.
public/assets/css/generalStyle.css
kauffj (Migrated from github.com) commented 2017-09-22 15:53:10 +02:00
Copy link

What if I'm not on Chrome?

What if I'm not on Chrome?
public/assets/js/authFunctions.js
kauffj (Migrated from github.com) commented 2017-09-22 15:53:58 +02:00
Copy link

Is this the only place we're making XMLHttpRequests in the app? If so, I suppose this is okay, if not, we might want to standardize this a bit.

Is this the only place we're making XMLHttpRequests in the app? If so, I suppose this is okay, if not, we might want to standardize this a bit.
views/partials/channelCreation.handlebars
kauffj (Migrated from github.com) commented 2017-09-22 15:56:10 +02:00
Copy link

Updating HTML via JS can quickly become a maintainability nightmare.

IMO a better design is to render all the elements and use JS to toggle hiding/showing, either by acting directly on the elements or changing the class name of the parent and using CSS to dictate what is shown.

Updating HTML via JS can quickly become a maintainability nightmare. IMO a better design is to render all the elements and use JS to toggle hiding/showing, either by acting directly on the elements or changing the class name of the parent and using CSS to dictate what is shown.
bones7242 (Migrated from github.com) reviewed 2017-09-25 17:49:10 +02:00
config/default.json
bones7242 (Migrated from github.com) commented 2017-09-25 17:49:10 +02:00
Copy link

👍

:+1:
bones7242 (Migrated from github.com) reviewed 2017-09-25 18:36:57 +02:00
config/default.json
@ -9,3 +10,2 @@
"Database": "lbry",
"Username": "none",
"Password": "none"
"Username": null,
bones7242 (Migrated from github.com) commented 2017-09-25 18:36:57 +02:00
Copy link

I did it so that when I called the variable later, if l logged the variable and got 'none' I knew the variable had been created but wasn't updated with the proper value for the environment. Not very clean/efficient. I fixed this now. They are all set to null and a script runs through them when the server loads and prints their values so I can check them up front. (250ead165b)

I did it so that when I called the variable later, if l logged the variable and got 'none' I knew the variable had been created but wasn't updated with the proper value for the environment. Not very clean/efficient. I fixed this now. They are all set to `null` and a script runs through them when the server loads and prints their values so I can check them up front. (https://github.com/lbryio/spee.ch/pull/170/commits/250ead165bd51c4b9812d514c724d320f526d90c)
bones7242 (Migrated from github.com) reviewed 2017-09-25 18:56:08 +02:00
controllers/publishController.js
bones7242 (Migrated from github.com) commented 2017-09-25 18:56:08 +02:00
Copy link

no, it will overwrite the first claim with the second, and just update the channel

no, it will overwrite the first claim with the second, and just update the channel
bones7242 (Migrated from github.com) reviewed 2017-09-25 20:26:11 +02:00
controllers/publishController.js
bones7242 (Migrated from github.com) commented 2017-09-25 20:26:11 +02:00
Copy link

👍

:+1:
bones7242 (Migrated from github.com) reviewed 2017-09-26 07:40:54 +02:00
models/user.js
@ -0,0 +1,29 @@
module.exports = (sequelize, { STRING }) => {
const User = sequelize.define(
'User',
bones7242 (Migrated from github.com) commented 2017-09-26 07:40:54 +02:00
Copy link

per our conversation today, I am updating the db to include both User and Channel tables

per our conversation today, I am updating the db to include both User and Channel tables
bones7242 (Migrated from github.com) reviewed 2017-09-26 07:53:17 +02:00
helpers/publishHelpers.js
@ -72,3 +70,3 @@
if (title === '' || title === null) {
if (title === null || title === '') {
title = name;
}
bones7242 (Migrated from github.com) commented 2017-09-26 07:53:16 +02:00
Copy link

👍

:+1:
bones7242 (Migrated from github.com) reviewed 2017-09-26 07:55:22 +02:00
helpers/handlebarsHelpers.js
@ -0,0 +8,4 @@
return new Handlebars.SafeString(
`<script>
(function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
(i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
bones7242 (Migrated from github.com) commented 2017-09-26 07:55:22 +02:00
Copy link

They have to be in a helper so that they can be generated based on the proper google analytics key from the environmental vars

They have to be in a helper so that they can be generated based on the proper google analytics key from the environmental vars
bones7242 commented 2017-09-29 02:28:48 +02:00 (Migrated from github.com)
Copy link

I still have yet to abstract the lbryApi into a proxy design, but the rest of the issues have been address. I'd like to commit this so that I can start working on the CSS on a separate branch. Will clean up the lbryApi in a separate PR.

I still have yet to abstract the lbryApi into a proxy design, but the rest of the issues have been address. I'd like to commit this so that I can start working on the CSS on a separate branch. Will clean up the lbryApi in a separate PR.
Sign in to join this conversation.
Reviewers
No reviewers
kauffj
Labels
Clear labels   
area: devops

   
area: discovery

   
area: docs

   
area: livestream

   
area: proposal

Work is part of a proposal

  
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
dependencies

Pull requests that update a dependency file

  
Epic

   
good first issue

   
hacktoberfest

Welcome to Hacktoberfest

  
help wanted

   
icebox

Long-term storage

  
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
on hold

Temporarily paused

  
Osprey

Spee.ch special project

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
protocol dependent

   
resilience

   
Tom's Wishlist

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: error handling

   
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
dependencies
Epic
good first issue
hacktoberfest
help wanted
icebox
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
Osprey
priority: blocker
priority: high
priority: low
priority: medium
protocol dependent
resilience
Tom's Wishlist
type: bug
type: discussion
type: error handling
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
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: LBRYCommunity/spee.ch#170
No description provided.
Delete branch "authentication"

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?