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

Add change logging to factoids #29

Closed
imbcmdth opened this issue Dec 3, 2013 · 9 comments
Closed

Add change logging to factoids #29

imbcmdth opened this issue Dec 3, 2013 · 9 comments
Assignees

Comments

@imbcmdth
Copy link
Member

imbcmdth commented Dec 3, 2013

We should record all changes to factoids in order to track and revert malicious or ill conceived edits and deletions.

My thinking is to add a new 'edits' array to each factoid that will contain change-object comprised of a nick, timestamp, edit type, and the previous value for each change. A new top level array will track factoid deletions and save the entire factoid along with a timestamp and nick.

I plan on unifying alias/factoid format by adding the 'editors' and 'creator' to aliases. This way factoids themselves function as a proper 'create' change record.

@ghost ghost assigned imbcmdth Dec 3, 2013
@imbcmdth
Copy link
Member Author

imbcmdth commented Dec 4, 2013

Here is a sample of my proposed *-factoids.json data format for change logging:

EDIT: Changed 'by' to 'editor' to have the JSON reflect 99f671a

{
    "wait": 30000,
    "factoids": {
        "bar": {
            "value": "i love bar",
            "creator": "ImBcmDth",
            "editors": [
                "ImBcmDth"
            ],
            "popularity": 0,
            "changes": [
                {
                    "date": "2013-12-04T01:54:49.470Z",
                    "editor": "ImBcmDth",
                    "old-value": "i am bar",
                    "new-value": "i love bar",
                    "regex": "s/am/love/"
                }
            ],
            "date": "2013-12-04T01:43:16.535Z"
        },
        "baz": {
            "value": "it was something else entirely",
            "popularity": 0,
            "editors": [
                "ImBcmDth"
            ],
            "changes": [
                {
                    "date": "2013-12-04T01:55:15.936Z",
                    "editor": "ImBcmDth",
                    "old-value": "i am baz",
                    "new-value": "it was something else entirely"
                }
            ]
        },
        "qux": {
            "alias": "bar",
            "popularity": 0,
            "editors": [
                "ImBcmDth"
            ],
            "changes": [
                {
                    "date": "2013-12-04T01:50:12.427Z",
                    "editor": "ImBcmDth",
                    "old-alias": "baz",
                    "new-alias": "bar"
                }
            ]
        }
    },
    "delete_log": [
        {
            "date": "2013-12-04T01:52:36.097Z",
            "editor": "ImBcmDth",
            "key": "foo",
            "value": {
                "value": "i am foo",
                "creator": "ImBcmDth",
                "editors": [],
                "popularity": 0,
                "changes": [],
                "date": "2013-12-04T01:43:05.424Z"
            }
        }
    ]
}

imbcmdth added a commit that referenced this issue Dec 4, 2013
@imbcmdth
Copy link
Member Author

imbcmdth commented Dec 4, 2013

I had to fix several "rough areas" for logging to make sense.

The major change is that you can no longer go from a factoid to an alias without first manually deleting the factoid (previously it would just silently overwrite the factoid with an alias.) Doing it this way forces you to create a delete record with the entire deleted factoid before worrying about adding an alias. It also allows you to have change-logs that differ slightly between factoids vs alias without worrying about recording some sort of "converted factoid to alias" change. I could automate the deletion but I think that may be better if a user has to force it explicitly.

The other minor change is that aliases now have all the same data as a factoid - popularity, creator, editors, and changes. The "unified factoid" object was mostly done just to simplify code by having fewer exception but it has the side effect of allowing you to track popularity of aliases separately from their targets.

Don't know if that will be useful but there is one more change to be aware of - calling a factoid through an alias will increment the popularity of both the alias and the factoid by 1. We may want to change this but I kind of like it. Previously, we didn't track popularity through aliases at all (not even for the alias target!)

@gkatsev
Copy link
Member

gkatsev commented Dec 4, 2013

Is there a way to unify by and editors and creator more? It's a bit of a pain that there are 3 ways to refer to them.

@ljharb
Copy link
Member

ljharb commented Dec 4, 2013

Would this be simpler to just store the factoids json in a git repo? Why reinvent the wheel?

@gkatsev
Copy link
Member

gkatsev commented Dec 4, 2013

Would this be simpler to just store the factoids json in a git repo? Why reinvent the wheel?

Have the latest change be the 'author' of the commit?

@imbcmdth
Copy link
Member Author

imbcmdth commented Dec 4, 2013

@gkatsev I left editors as is because I thought it might be nice if we ever were to ever make a web-based factoid viewer (wink, wink). Instead of having to trawl the change log of each factoid to pull out unique names for a vanity "Editors" display, the data is already right there. I will admit it is redundant but who cares about data normalization? ;)

IMO, creator is necessary because the factoid itself is the creation record (that is why I added a date field to the factoid proper). Also, it already exists in many of the factoids so changing that property to by creates.. problems.

The idea is to be a transparent (to @inimino) drop-in change. There is no conversion script - it should just work with the existing factoids (which seem to have 2 - 3 slightly different formats).

@gkatsev
Copy link
Member

gkatsev commented Dec 4, 2013

@imbcmdth yeah, creator is necessary, I guess editors and by is annoying. Perhaps the outer one should be editors and then each change would have an editor?

@ljharb
Copy link
Member

ljharb commented Dec 4, 2013

@gkatsev instead of storing an "editors" array, we could just store a "last author" field separate from "creator", and the git repo would track the changes.

@gkatsev
Copy link
Member

gkatsev commented Dec 4, 2013

I guess we could do the same even with the changes field in the proposed json above.

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

No branches or pull requests

3 participants