Skip to content

Conversation

@smalinin
Copy link
Contributor

@smalinin smalinin commented Jun 8, 2018

No description provided.

@RubenVerborgh
Copy link
Contributor

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

Also, the build is currently broken because of

/home/travis/build/solid/node-solid-server/lib/requests/login-request.js:175:11: Infix operators must be spaced.

@smalinin
Copy link
Contributor Author

smalinin commented Jun 8, 2018

It looks, that only the next fixes were added

  • fix account-template.js
    • for use both externalWebID and external Delegate
    • for add oidcIssuer relation to card
  • fix Login - it doesn't work properly with external WebID
  • fix account template test for work with updated function templateSubstitutionsFor()

outliner.GotoSubject(subject, true, undefined, true, undefined);

// Authenticate the user
UI.authn.checkUser()
Copy link
Contributor

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

Copy link
Contributor Author

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

@smalinin any particular reason for all the chmod 100644 → 100755 changes?

tlsCertExponent: userAccount.tlsCertificateExponent,
addOwlSameAs
addOwlSameAs,
serverUri: accountManager.host.serverUri
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

if (this.clientId) {
let uri

if (this.authQueryParams['client_id']) {
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@dmitrizagidulin
Copy link
Contributor

@smalinin Thanks for the fixes! There's still 3 .acl files with chmod changes, but overall 👍 from me!

@smalinin
Copy link
Contributor Author

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

@smalinin sure thing! (you can see them towards the top of the Files Changed tab)

  • default-templates/new-account/.acl
  • default-templates/new-account/.meta.acl
  • default-templates/new-account/inbox/.acl

@smalinin
Copy link
Contributor Author

smalinin commented Jul 9, 2018

@dmitrizagidulin Ok, I see, it looks that in my prev PR for Node-solid-server, I changed chmod for these files to rwxrwxr-x mode, and this was missed in prev PR review stage. Now I set chmod to rw-rw-r-- for all .acl files, that is the properly value.

@dmitrizagidulin
Copy link
Contributor

No prob. Thanks again!

@RubenVerborgh
Copy link
Contributor

Reminder that we should squash this into one commit, as 41d9bb5 is problematic (not a merge commit, will mess with our history).

@kidehen
Copy link

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:

  1. Actions Items?
  2. Action item ownership?

@RubenVerborgh
Copy link
Contributor

Actions Items?

Whoever merges this, should squash merge, not create a merge commit.

Action item ownership?

The merger.

@RubenVerborgh
Copy link
Contributor

Assigning this to @kjetilk for now, as a reminder we need to look into this.

@RubenVerborgh RubenVerborgh assigned kjetilk and justinwb and unassigned kjetilk Jul 19, 2018
@RubenVerborgh
Copy link
Contributor

@smalinin @kidehen We have discussed this, and @justinwb will be following up on this PR with you. Thanks for your patience.

@smalinin
Copy link
Contributor Author

smalinin commented Jul 20, 2018

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

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

kidehen commented Jul 29, 2018

@justinwb,

What's happening here?

@justinwb
Copy link
Contributor

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

smalinin commented Jul 30, 2018

@RubenVerborgh I have recreated PR #728 with fresh fork and collect all changes to one commit.

@RubenVerborgh RubenVerborgh changed the base branch from v4.1.0 to feature/webid-delegation August 10, 2018 21:43
@RubenVerborgh
Copy link
Contributor

Note, the branch has been renamed into feature/webid-delegation, since this will not be part of v4.1.

@RubenVerborgh
Copy link
Contributor

Following up in #728.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants