- Notifications
You must be signed in to change notification settings - Fork 305
Updated pull request based on branch V4.1.0 from solid/node-solid-server#701
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
Updated pull request based on branch V4.1.0 from solid/node-solid-server #701
Uh oh!
There was an error while loading. Please reload this page.
Conversation
- fix code style for camelCase - fix code style for remove semicolons - fix tests - use rdflib for WebID+Delegate
- register created wrong card file, because renegotiate TLS was missied
Sync with branch V4.1.0 from solid/node-solid-server
RubenVerborgh commented Jun 8, 2018
@smalinin Thanks, could you summarize the changes? Note: lets make this a squash merge, because of the difficult history tail (with "sync" commits instead of merge commits). |
RubenVerborgh commented Jun 8, 2018
Also, the build is currently broken because of |
smalinin commented Jun 8, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
It looks, that only the next fixes were added
|
| outliner.GotoSubject(subject, true, undefined, true, undefined); | ||
| // Authenticate the user | ||
| UI.authn.checkUser() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the authn.checkUser() call here, in databrowser? It's currently necessary..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed the above.
dmitrizagidulin commented Jun 8, 2018
@smalinin any particular reason for all the |
lib/models/account-template.js Outdated
| tlsCertExponent: userAccount.tlsCertificateExponent, | ||
| addOwlSameAs | ||
| addOwlSameAs, | ||
| serverUri: accountManager.host.serverUri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the accountManager is only being used to get the host instance, maybe just pass in host directly? Instead of the full accountManager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lib/requests/login-request.js Outdated
| if (this.clientId){ | ||
| let uri | ||
| if (this.authQueryParams['client_id']){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change from this.clientId to this.authQueryParams['client_id'] here? Was clientId not being properly initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Remove handle for /logout and /goodbye
dmitrizagidulin commented Jul 9, 2018
@smalinin Thanks for the fixes! There's still 3 .acl files with |
smalinin commented Jul 9, 2018
@dmitrizagidulin Could you write me the filenames with a problem in chmod? I have rechecked all files and I could not found such issue! |
dmitrizagidulin commented Jul 9, 2018
@smalinin sure thing! (you can see them towards the top of the Files Changed tab)
|
smalinin commented Jul 9, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@dmitrizagidulin Ok, I see, it looks that in my prev PR for Node-solid-server, I changed chmod for these files to |
dmitrizagidulin commented Jul 9, 2018
No prob. Thanks again! |
RubenVerborgh commented Jul 9, 2018
Reminder that we should squash this into one commit, as 41d9bb5 is problematic (not a merge commit, will mess with our history). |
kidehen commented Jul 19, 2018
@RubenVerborgh@dmitrizagidulin , What does "Reminder that we should squash this into one commit, as 41d9bb5 is problematic (not a merge commit, will mess with our history)." mean in regards to:
|
RubenVerborgh commented Jul 19, 2018
Whoever merges this, should squash merge, not create a merge commit.
The merger. |
RubenVerborgh commented Jul 19, 2018
Assigning this to @kjetilk for now, as a reminder we need to look into this. |
RubenVerborgh commented Jul 19, 2018
smalinin commented Jul 20, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@RubenVerborgh Basically I could create fork from current state on solid:v4.1.0 , apply all changes above and create a new PR(it is not problem). Will it be better for you ? |
RubenVerborgh commented Jul 20, 2018
@smalinin That would help. Note that a new PR is not needed for that, you can just force push to the branch (but I don't mind either). |
kidehen commented Jul 29, 2018
What's happening here? |
justinwb commented Jul 29, 2018
@kidehen just left you a note on Gitter. Summation - we’ve got it on the backlog to merge as soon as we wrap a couple more priority items for the 5.0 release. One thing that would help speed things up a ton when we merge is if the contributors in that pull can go through and add as many comments as possible ahead of time so we’re not going back and forth with a ton of questions |
smalinin commented Jul 30, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@RubenVerborgh I have recreated PR #728 with fresh fork and collect all changes to one commit. |
RubenVerborgh commented Aug 10, 2018
Note, the branch has been renamed into feature/webid-delegation, since this will not be part of v4.1. |
RubenVerborgh commented Aug 10, 2018
Following up in #728. |
No description provided.