cla-assistant / cla-assistant

Contributor License Agreement assistant (CLA assistant)
https://cla-assistant.io
Apache License 2.0
1.34k stars 262 forks source link

Store more information about the CLA signer #64

Closed sdirix closed 8 years ago

sdirix commented 9 years ago

The CLA assistant really helps to deal with CLA signments for GitHub projects in an easy and intuitive way.

Currently the information collected about the CLA signer is limited to the GitHub username. This could lead to traceability problems when a contributer deletes his GitHub account or does not use GitHub anymore. Also some organisations request more information about contributors in general.

Therefore I wanted to ask if you are interested in extending the signment process by collecting more information about the user via a form (name, address, email, phone etc.) or via the GitHub-API querying the signer's GitHub profile. This data could then easily be stored in some additional fields of the mongo-db.

KharitonOff commented 9 years ago

Thank you for your proposal, Stefan. We have created CLA assistant with the intention to simplify the contribution process and make it for the contributors as easy as possible to sign a CLA. Providing any kind of a form would reduce the simplicity for the enduser. I think it's a good Idea to query the GitHub profile.

reem commented 8 years ago

I am also considering using cla-assistant for an open source project at work, and storing emails, names, or any other information available other than just github username would be a wonderful feature and would make it much easier to adopt cla-assistant in many organizations.

dcrosta commented 8 years ago

@KharitonOff What if the CLA gist could contain some metadata about additional required fields that each organization wants to collect -- e.g. email address or full/legal name. It wouldn't have to be automatically enabled for all CLA-Assistant users (satisfies the streamlining criteria), but places that want this extra information could collect it.

KharitonOff commented 8 years ago

Thank you for your proposal, we are thinking in the same direction: provide an opportunity to add required fields... We'll take your Idea to use metadata in gist in consideration. Do you have any example or ideas how it could look like?

dcrosta commented 8 years ago

@KharitonOff something like:

<text of the CLA here>

----

Your Name*:
Your Email Address*:
Your Employer's Name:
[ ] I have the right to assign copyright for contributions to this project*

Which would cause 3 text fields to be created, 2 of them required (the ones with *s), and one required checkbox.

My only concern would be what text that might trigger this formatting may already exist in CLAs out there. If I were someone using CLA Assistant, I wouldn't want my CLA form changing out from under me. Is there a way to grandfather in existing CLAs, and have this feature only apply to newly created ones?

thojansen commented 8 years ago

@dcrosta right now, I think we would rather provide a fixed set of predefined (optional) fields instead of mixing that with the CLA text itself (which might cause some problems). The assumption right now is that these optional fields would be more or less similar for different projects. Feedback appreciated.

sdirix commented 8 years ago

Thanks for looking into this issue.

Fields I would like to suggest:

Offering predefined fields is probably the easiest solution to implement. However I would like to suggest to be able to add an own description to each field or at least to the whole form. For example the amount of information needed regarding the employer might differ from project to project.

If you are looking for a generic solution covering all use cases you could take a look at JSONSchema. The user could then add an own schema to each CLA (for example via Gist) from which each form is generated.

Example Schema:

{
    "title": "Example Form",
    "type": "object",
    "properties": {
        "firstName": {
            "type": "string"
        },
        "lastName": {
            "type": "string"
        },
        "age": {
            "description": "Age in years",
            "type": "integer",
            "minimum": 0
        }
    },
    "required": ["firstName", "lastName", "age"]
}
dcrosta commented 8 years ago

From our attorney, these are the fields we'd want in addition to the general CLA agreement:

  1. Full Name
  2. Email Address
  3. Name of any entities with rights to the contribution (e.g. your employer)
  4. A checkbox with "I have read and agree to the CLA"

Re #3, our CLA says "If Your contribution is on behalf of an entity (such as your employer), or if there is an entity that has rights to your contribution ...", so we want to know who those entities are.

Re #4, we believe that having to take a separate action -- checking the checkbox -- is a stronger sign of commitment than just clicking the big button (a habit that many of us are in, just to get the message to go away).

Numbers 1, 2, and 4 should be required (or we should have the option to make them required)

As I indicated before, ideally, we'd like to be able to customize the text along with these extra fields, as well as which fields appear. I still think that having some kind of metadata with the CLA, or possibly in a separate gist, will be the most general and useful way to support this feature; but if instead it ended up as some configuration done through the CLA Assistant webapp for each repo, that would be fine for us too.

mmoayyed commented 8 years ago

Just adding a bit of feedback in support of this issue:

We are hoping to use the CLA assistant with this project: https://github.com/apereo/cas

Once of the concerns here is that the click-thru functionality as presented here does not or may not quite pass the legal muster, and so if we could design the process in such a way where additional data is collected from the contributor at least in the form of name and email address that would be great.

ahicks92 commented 8 years ago

I'm also looking at using CLA Assistant with agreements from Harmony Agreements.

When you generate these agreements, they specifically want name and address. I am nervous about removing these fields because I am not fortunate enough to have easy access to legal advice.

It would be really helpful if this were given priority.

lipis commented 8 years ago

We are willing to test as soon as it's out :)

KharitonOff commented 8 years ago

@sdirix, @reem, @dcrosta, @mmoayyed, @camlorn, @lipis, @matz3, @jdanyow, @ejsmith, @niemyjski We are almost done! 🎉 Now we need your help! It would be great if you could test the current version and give us your feedback:

You can try the current state on our staging instance: https://preview.cla-assistant.io/

What's in:

Thanks a lot for your valuable feedback and for your support!

lipis commented 8 years ago

@KharitonOff Awesome stuff!

What about radio buttons? We could easily describe them as well if you are not supporting them at the moment with something like that:

...
"category": {
    "title": "How do you sign?",
    "type": "radio",
    "items": [
        {
            "name": "myself",
            "title": "I am signing on behalf of myself."
        },
        {
            "name": "employer",
            "title": "I am signing on behalf of my employer."
        }              
    ]
}
...
lipis commented 8 years ago

We could also provide another field called description that could look something like this:

screen shot 2016-08-01 at 12 23 06
lipis commented 8 years ago

I created a dummy projects for testing it this and it looks my "testers" are getting 401 :) https://github.com/lipis/cla (anyone is welcome to create a new PR to see it action..)

lipis commented 8 years ago

It looks like you have to be logged in to https://preview.cla-assistant.io before signing (if it's just part of the preview then it's totally fine).

KharitonOff commented 8 years ago

@lipis : 👍 thank you very much for testing and issue reporting! I've fixed the logging issue. Now the signer should see the CLA without logging into the tool.

Radio-buttons should work now too. I changed your proposal to following schema:

{
    "title": "Custom Fields",
    "type": "object",
    "properties": {
        ...
        "category": {
            "title": "How do you sign?",
            "type": { 
                "enum": [ 
                    "I am signing on behalf of myself.",
                    "I am signing on behalf of my employer."
                ]
            }
        }
    },
    "required": ["email", "category"]
}

The 'description' attribute is currently used for tool-tip... I'll discuss with our designer your suggestion.

lipis commented 8 years ago

Good stuff! The schema is even simpler so that's perfect..

As for the description don't forget the mobile users.. one design to rule them all.. simple text as suggested is more straightforward as people have to read the whole thing anyway since it's a CLA.. Plus it will be clearer to make as less mistakes as possible.

dcrosta commented 8 years ago

This looks great -- many thanks! Any idea when it will be released to the production CLA-Assistant?

sdirix commented 8 years ago

Thank you very very much for implementing this feature!

I really like that you went with the flexible route of allowing users to create form descriptions. I tested the preview and it works as expected! Great!

As @lipis already mentioned, I would also strongly prefer that the 'description' is rendered as text above or below the input.

Currently when the metadata is invalid, the user only sees an error message which looks like this:

Error. There is no CLA to sign for . (Unexpected string at....)

I would like to suggest to catch errors like this (see also my next point at how to do it) and then still display the CLA but with an additional info that the form could not be rendered and the user should contact the repository owner to sort the problem out.

My greatest remark is about your use of JSONSchema. You have mainly two options to support JSONSchema:

  1. You could support it as a means for the users of cla-assistant to define the data they expect from their signers. Cla-assistant then uses the schema of the user to generate a form. This was the way I proposed in my previous comment.
  2. You could also use JSONSchema to define a schema yourself of the data you want to receive from the cla-assistant user to generate the form and database entries.

What you currently do is to define an own schema format which looks similar to JSONSchema but is actually incompatible. You can check this with the validator I linked below.

If you want to go the route of the first option, then you let the user define their own schema, read in the schema and generate from it the form and database entries.

However I guess the second option is actually closer to the workflow you currently expect. Your schema and an example schema instance (the 'metadata' file) could then look like this:

{
    "type": "array",
    "items": {
        "type": "object",
        "properties": {
            "title": {
                "type": "string"
            },
            "type": {
                "anyOf": [
                    {
                        "type": "string"
                    },
                    {
                        "type": "object",
                        "properties": {
                            "enum": {
                                "type": "array",
                                "minItems": 2,
                                "items": {"type": "string"},
                                "uniqueItems": true
                            }
                        },
                        "required": ["enum"]
                    }
                ]
            },
            "required": {
                "type": "boolean"
            },
            "description": {
                "type": "string"
            },
            "githubKey": {
                "type": "string"
            }
        },
        "additionalProperties": false,
        "required": ["title", "type"]
    }
}

Explanation: You expect an array of objects. These objects must adhere to the the type you defined in items: They must have a title and a type. The title must be a string. The type can either be a string or an object with the properties enum which is an array of strings. Other fields can also be added but they are optional: The boolean required, the string description and the string githubkey.

This means the data in the metadata gist should then look like this:

[
    {
        "title": "Full Name",
        "type": "string",
        "githubKey": "name",
        "required": true
    },
    {
        "title": "Email Address",
        "type": "string",
        "githubKey": "email",
        "required": true
    },
    {
        "title": "Mailing Address",
        "type": "string"
    },
    {
        "title": "Age",
        "type": "integer"
    },
    {
        "title": "Corporate Information",
        "type" : {
            "enum": [
                "I am signing on behalf of myself.",
                "I am signing on behalf of my employer."
            ]
        },
        "required": true
    }
]

One advantage of this approach is, that you can for example validate the 'metadata' gist once the user links it on the dashboard.

There exist many validators for JSONSchema. Here is an example of an online validator: jsonschemavalidator. Copy my example schema to the left and the example metadata to the right and you will see that it successfully validates. When you enter invalid data you immediately get a nice error message.

Since you did not yet introduce the feature into the productive build I would strongly suggest to define a clean json schema (you can use mine if you want) before you get any legacy problems. You can of course also use your current (informal) definition, but since it closely resembles what the JSONSchema specification itself looks like it has a a lot of overhead.

Thank you again for your great work! Once this feature is integrated there should be no more obstacles to finally use the cla-assistant within our projects!

KharitonOff commented 8 years ago

Ok, I see! Thank you for pointing out my misunderstanding! 😏 And yes, I prefer creating our own Schema. I've described it now, based on your suggestion:

{
  "title": "JSON Schema for CLA assistant custom fields",
  "$schema": "http://json-schema.org/draft-04/schema#",
  "type": "object",
  "patternProperties": {
    "^[A-Za-z0-9]*$": {
      "type": "object",
      "description": "Each property describes additional data which should be collected from a CLA signer and generates a form field",
      "properties": {
        "title": {
          "type": "string",
          "description": "Title is used to create a label for the form field"
        },
        "type": {
          "oneOf": [
            {
              "enum": ["string", "number", "boolean"],
              "description": "Field of type boolean generates a checkbox"
            },
            {
              "type": "object",
              "description": "Field of type enum generates radio buttons",
              "properties": {
                "enum": {
                  "type": "array",
                  "minItems": 2,
                  "items": {
                    "type": "string"
                  },
                  "uniqueItems": true
                }
              }
            }
          ]
        },
        "required": {
          "type": "boolean",
          "description": "Requiered fields are marked with asterisk and must be filled by signer"
        },
        "description": {
          "type": "string",
          "description": "Field description generates small text behind the label"
        },
        "maximum": {
          "type": "number",
          "description": "Can be used for fields of type number"
        },
        "minimum": {
          "type": "number",
          "description": "Can be used for fields of type number"
        },
        "githubKey": {
          "type": "string",
          "description": "Data of github user profile can be used to prefill the form. GithubKey refers to the response of the github api https://developer.github.com/v3/users/#get-a-single-user"
        }
      },
      "additionalProperties": false
    }
  },
  "additionalProperties": false
}

The metadata json could look like this one:

{
  "name": {
    "title": "Full Name",
    "type": "string",
    "githubKey": "name"
  },
  "eMail": {
    "title": "E-Mail",
    "type": "string",
    "githubKey": "email",
    "required": true
  },
  "age": {
    "title": "Age",
    "description": "Age in years",
    "type": "number",
    "minimum": 18
  },
  "agreement": {
    "type": "boolean",
    "title": "I have the right to assign copyright for contributions to this project"
  },
  "category": {
    "title": "How do you sign?",
    "type": {
      "enum": [
        "I am signing on behalf of myself.",
        "I am signing on behalf of my employer."
      ]
    },
    "required": true
  }
}

@lipis , @sdirix : regarding description: we have decided to put the description in the input field (in sense of consistency) like we have it in the "link screen" : bildschirmfoto 2016-08-03 um 12 51 56 I hope it's ok for you.

lipis commented 8 years ago

Just my 50cents.. the placeholder is usually used for some example text and it will go away right after the first character or some possible autocomplete forms and the end user will never going to see the possible instructions that could be easily missed if they "know" how it should be :D

Instructions should be always visible for clarity.

KharitonOff commented 8 years ago

@lipis ok, thank you for your input! In your proposal the space between the fields with and without description would be unequal. We've placed the description out of the input field, behind the label.

bildschirmfoto 2016-08-04 um 10 56 41

lipis commented 8 years ago

Looks pretty good!! Thank you..

dcrosta commented 8 years ago

@KharitonOff this is all looking very very good :) Any idea when it will be released to production?

KharitonOff commented 8 years ago

@dcrosta we are going to release it next week 🙂

thojansen commented 8 years ago

We decided to release v1.4 today to production :shipit:. Thanks for testing everything and giving us valuable feedback. Let us know in case you have any problems.