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

Manage collaborators #24

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Manage collaborators #24

wants to merge 33 commits into from

Conversation

git-voo
Copy link
Contributor

@git-voo git-voo commented Sep 4, 2024

Implemented

  • View the list of contributors
  • invite a new member
  • remove member (from UI)
  • error display on UI
  • permissions:
    • only the Owner and Leader can view the remove button
    • only the Owner and Leader can add new members

Pending Implementation

  • - remove member from DB
  • - revoke member role
  • move base URL to central point (env?)
  • move add/remove member functions to Project class
  • move getProject method to Project class

Create New Issue For

  • populate the Collaboration tab with Project title and group title
  • Populate project list with DB data

@git-voo git-voo requested a review from cubap September 4, 2024 16:17
@git-voo git-voo self-assigned this Sep 4, 2024
Copy link
Member

@cubap cubap left a comment

Choose a reason for hiding this comment

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

Some comments. I'll let you finish it.

manage/collaboration.html Outdated Show resolved Hide resolved

let isOwnerOrLeader = false;
let projectID;
let testToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6Ik9FVTBORFk0T1RVNVJrRXlOREl5TTBFMU1FVXdNMFUyT0RGQk9UaEZSa1JDTXpnek1FSTRNdyJ9.eyJodHRwOi8vc3RvcmUucmVydW0uaW8vYWdlbnQiOiJodHRwczovL3N0b3JlLnJlcnVtLmlvL3YxL2lkLzY1Zjg2MTVlYzQzYmQ2NjU2OGM2NjZmYSIsImh0dHA6Ly9yZXJ1bS5pby9hcHBfZmxhZyI6WyJ0cGVuIiwiZGxhIl0sImh0dHA6Ly9kdW5iYXIucmVydW0uaW8vYXBwX2ZsYWciOlsidHBlbiIsImRsYSJdLCJodHRwOi8vcmVydW0uaW8vdXNlcl9yb2xlcyI6eyJyb2xlcyI6WyJkdW5iYXJfdXNlcl9wdWJsaWMiLCJnbG9zc2luZ191c2VyX3B1YmxpYyIsImxyZGFfdXNlcl9wdWJsaWMiLCJyZXJ1bV91c2VyX3B1YmxpYyIsInRwZW5fdXNlcl9hZG1pbiIsInRwZW5fdXNlcl9pbmFjdGl2ZSJdfSwiaHR0cDovL2R1bmJhci5yZXJ1bS5pby91c2VyX3JvbGVzIjp7InJvbGVzIjpbImR1bmJhcl91c2VyX3B1YmxpYyIsImdsb3NzaW5nX3VzZXJfcHVibGljIiwibHJkYV91c2VyX3B1YmxpYyIsInJlcnVtX3VzZXJfcHVibGljIiwidHBlbl91c2VyX2FkbWluIiwidHBlbl91c2VyX2luYWN0aXZlIl19LCJpc3MiOiJodHRwczovL2N1YmFwLmF1dGgwLmNvbS8iLCJzdWIiOiJhdXRoMHw2NWY4NjE1ZDZjNmJlYjIzMTVjZWY4MjIiLCJhdWQiOlsiaHR0cHM6Ly9jdWJhcC5hdXRoMC5jb20vYXBpL3YyLyIsImh0dHBzOi8vY3ViYXAuYXV0aDAuY29tL3VzZXJpbmZvIl0sImlhdCI6MTcyNTQ1ODg1NywiZXhwIjoxNzI1NDY2MDU3LCJzY29wZSI6Im9wZW5pZCBwcm9maWxlIGVtYWlsIHVwZGF0ZTpjdXJyZW50X3VzZXJfbWV0YWRhdGEgb2ZmbGluZV9hY2Nlc3MiLCJhenAiOiJiQnVnRk1XSFVvMU9oblNaTXBZVVh4aTNZMVVKSTdLbCJ9.D06LUWxeS2HKibY3sLxe18f7n3b56PifEGaoGriPDqkvsIG077Sa9qn-6WZCguw3TFSGKN0GG6YM9Gd8hGdXkPB3gEpYSlAzwZwHZzvs3bV1dPBE6zSkL1Z_lTyVXyhjh13oCJx73Y66DjjZkuLGvVG7_IEC7OF9njYAF5MgqgvyQZlSTKpOnxgu76S7SNjrecnMscMLNASe6fDSeH7NUXP3HEhYuy1dbBV_pVaxFZLIjx5-KHNCHs4LvuybSmm0GWt6HyRTpi69AsA_rSGDcW9rzLZSCGRWr1fN4WOWKbM-Lwpdt_0ir_djQmbkrnk9sO4FQ7P5ZmQD_vAoPTPGWQ"
Copy link
Member

Choose a reason for hiding this comment

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

okay for a test, but for the PR, this needs to use the auth and login so that it is a portable interface.

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 may need to work with you on that @cubap I'm still getting a 400 from auth0 on login/logout from interfaces

collaborators/index.html Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

These do not really need to look like TPEN28 and it might be harder to reskin it later if it has more than what it needs to be basically functional.

collaborators/index.mjs Outdated Show resolved Hide resolved
collaborators/index.mjs Show resolved Hide resolved
collaborators/index.mjs Outdated Show resolved Hide resolved
collaborators/index.mjs Outdated Show resolved Hide resolved
css/collaborators/index.css Outdated Show resolved Hide resolved
css/collaborators/index.css Outdated Show resolved Hide resolved
@cubap
Copy link
Member

cubap commented Oct 25, 2024

so much good work here, but I think we need to eliminate some of the redundancies. You've already got this instinct. I'm seeing some things that are in /iiif-tools and the auth struggles should be handled already. I'm going to go PR something real quick...

Copy link
Member

Choose a reason for hiding this comment

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

This directory shouldn't be versioned

cubap and others added 3 commits November 4, 2024 20:26
* logout redirect hotfix

* logout redirect hotfix

* Tpen class (#36)

* Update index.mjs

bad merge added local env

* Create index.mjs

* TPEN class with authentication

* match docs to class

* swap out auth

* adding expiry

* cleaning up the auth

* Add tpen class auth to transcription (#37)

* replacing URLsafed chars with base64 equiv

* working with new auth

* removing excess

* error handling

* moving the file to a Class

* Starting here.

* auth is for TPEN

* userMessages

* notes

* get User out of TPEN

* collaborators updated.

---------

Co-authored-by: Bryan Haberberger <[email protected]>
@git-voo git-voo marked this pull request as ready for review November 11, 2024 16:54
@git-voo git-voo requested a review from cubap November 11, 2024 16:54
TPEN/index.mjs Outdated Show resolved Hide resolved
TPEN/index.mjs Outdated Show resolved Hide resolved
TPEN/index.mjs Outdated
@@ -41,8 +40,8 @@ export default class TPEN {
}

set currentUser(user) {
// confirm user is a User object
if (!user.displayName || !user._id) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this || is so an unloaded User is never entered in as "#currentUser"

TPEN/index.mjs Outdated Show resolved Hide resolved
TPEN/index.mjs Outdated Show resolved Hide resolved
_site/api/project.mjs Show resolved Hide resolved
api/project.mjs Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
import getActiveProject from "../utilities/getActiveProject.mjs"
import getHash from "../utilities/getHash.mjs"


async function fetchProjects() {
Copy link
Member

Choose a reason for hiding this comment

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

seems like a chance to use the Project Class you made.

utilities/checkUserAuthentication.mjs Outdated Show resolved Hide resolved
Copy link
Member

@thehabes thehabes left a comment

Choose a reason for hiding this comment

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

It would be nice if the projects in the project list were clickable to load them up by their id.

Noting it is not possible to delete an item in the project list. I was informed the API is not ready for that yet, so this makes sense.

In user management I am not able to remove members from a group or change their roles (as an OWNER and/or a LEADER)

In user management I get a conflicting message when I try to add/invite a new member. The Network says 403 Forbidden. Pagination says other things.
image

/components/user-profile does not load, but the index.html component does.
image

/components/manage-collaborators is empty and should probably be removed to avoid confusion.

I am suspicious of seeing the instances of these two things together...is one of them wrong

  • TPEN_USER.authentication
  • TPEN_USER.authorization

This may allude to a need for a small "how do we get the bearer token" refactor in files that have not been adjusted for the TPEN and User class stuff. The approach does not appear to be synchronized across the files.

manage/index.mjs Show resolved Hide resolved
@@ -28,7 +28,7 @@ export default class ProjectsList extends HTMLElement {
if (oldValue !== newValue) {
const loadedUser = new User(newValue)
loadedUser.authentication = TPEN.getAuthorization()
Copy link
Member

Choose a reason for hiding this comment

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

authentication vs authorization. There are instances where this is set like TPEN_USER.authorization. It looks like a difference between TPEN.getAuthorization and checkUserAuthentication and maybe this is right.

Copy link
Member

Choose a reason for hiding this comment

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

/components/user-profile does not load, but the index.html component does.

fix posted to watch for user-id change if it is the only component on the page

Copy link
Member

Choose a reason for hiding this comment

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

getAuthorization() is for consistency from TPEN_USER. Strictly speaking, we are getting the idToken, so a refactored name makes sense. That said. This isn't the place for it. Someone can mention this in a new issue, I suppose.

@git-voo
Copy link
Contributor Author

git-voo commented Nov 25, 2024

The UI update for addMember is currently being delayed because of the delayed/no response from the SMTP server since it sends emails. It'd be fixed once the server is back up.

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

Successfully merging this pull request may close these issues.

3 participants