riannucci / rietveldv2 Goto Github PK

At the very least, we need to prevent common 'double-clicking' problems. Right now on stock Rietveld, if you e.g. fill in a Reply and then spam click the 'send' button, you'll end up with a bazillion Messages (and emails to go with them).

Simple typos, etc. are often more convenient to fix right when you read them. There's nothing stopping us from building the web interface so that you can 'modify' the right-hand side of the patch and then create a new patchset on the same issue which contains the new right-hand side (because we're generating the diff on the server, instead of generating the new file).

Right now you have to pre-set a column width. There could easily 2 settings:

• Column width [60 80 100 120 ...]
• Strict Column width [true false]

In non-strict mode, it would size to the file (and show a column marker at 'Column width). Otherwise it would behave as it does today (force wrapping).

Basically, we want people who are /reviewers/ to be able to do a limited amount of modification to the Issue (update modified time, add Messages/Comments). Currently this triggers the 'update' auth call, which is supposed to just be for modifying the 'core' fields.

Maybe we need 2-tiers of Issue metadata:

• per-user Issue metadata (we have this for drafts, starred, etc.)
• per-Issue Issue metadata (could store last_message, modification time, reviewers, cc, etc.)
  • This would solve the problem of allowing people cc'd to add more cc's and reviewers, without giving them a carte blanche to update the description (thus allowing them to add COLLABORATORS), or add patchsets.

Viewers of the Issue would have update on the per-Issue metadata (and their own per-user metadata, of course), but wouldn't have update on the actual Issue (only the owner + collaborators would have that).

Then when we're using mark_async(), we'll have to use it to modify the metadata object instead. Maybe we could use the following hierarchy:

• Issue
  • Patchset
  • Meta
    • Messages

Meta would logically live 'side-by-side' with the Issue (i.e. it would have a fixed id, there would only ever be one of them).

It should be possible to build uploadv2.py such that the tests can (as HttpTests):

• Import it as a module (thus, its code will be included under coverage)
• Generate a mock git / svn / hg repo
• Execute the uploadv2.py guts providing the HttpTest api as its url-fetcher interface.

This will allow the uploadv2.py client + server code be tested in the same context with coverage without needing to write specific detailed assert-based unit tests (though those should be added to provide coverage for hard-to-execute code paths).

The format of the tests wouldn't need to change (except that 'login'/'logout' would need to be more sophisticated). The test expectations would obviously be different, but they should match ~90%+.

We'll need to do some work so that the old and new Accounts are harmonized. This may mean backporting some of the new fields to the old model, and then auto-converting the old model to the new one in the old code.

Then we can map-reduce to convert all the old Accounts to compatible new Accounts.

One of the fails in the old model is that starred issues are kept as a list on the old Account. In the new model, they're stored as a boolean in the entity (Account, X, IssueMetadata, ).

May be off-by-one, not sure (see comments_httptest.py)

I think it will be necessary in the near future to have per-user themes. This isn't as daunting as It would seem, since I've already decreed (and no one has objected 😄 ) that all write-access to the datastore will happen through the API. Additionally, since the API is versioned, and we have pretty easy ways to test HTTP endpoints in different conditions, it would be trivial to extend the api to support new features, while allowing the old api to continue supporting old themes, as necessary.

I see at least 2 themes that will be necessary in the near future

• Oldschool (current look and feel of rietveld)
• Angular.js (jparent is planning on doing an angular UI for rietveld)

In addition to being selectable on a per-user basis, it would be great if themes had a way to expose options on a per-theme basis which showed up in the user settings. In particular, that would have made the introduction of colored reviewers and bolded 'activity' to the current theme easier (since there would be a way to opt-out or opt-in). In the current rietveld, there was essentially no easy way to do that, which lead to a lot of grief on various mailing lists.

In the current model, we have one UI which essential never changes because people are afraid of change. I would like to break this habit so that rietveld's UI could undergo continuous evolution, but I also respect the need of users for some stability in their UI.

Since all Themes are implemented against the API, the API should remain fairly stable (or stable core, plus optional extensions), and the UI is selected at request-time, it shouldn't be too hard to support multiple UIs. I would propose we have something like:

• 'classic' - what we have today, no changes
• 'modern-stable'/'modern-beta'/'modern-canary' - These are the 'modern' theme, and follow some revolving N-week process for rolling one to the other. If you want to live on the wild-side, stay on modern-canary, if you want new stuff, but actually plan on using rietveld ever, modern-beta is good, and if you want new stuff but never want to break, stick to modern-stable.
• 'angular-stable'/... - Different UI concept, same basic pattern of release
• etc.

This should allow new development to happen in a controlled way, with developers getting to chose their destiny at least somewhat. At some point, it would probably make sense to fork modern into just 'modern', and then start a new 'modern.v2-*' series, etc.

It may also be possible to just have canary/stable (no beta).

We need the ability to do a couple internal different things with routes which I don't think django's urlpatterns will suffice for. In particular, I think the high level logic should go something like:

def high_level_router(request):
  if request.url.matches('codereview/api/.*'):
    api_router.route(request)
  # this code can go away after fully migrating to new schema
  if request.url.matches('(\d+)/.*'): 
    issue = ndb.Key('Issue', request.url.match_group(1)).get()
    if issue.VERSION is None:
      old_router.route(request)
  account = auth_models.current_account()
  theme = theme_loader.load(account.theme)  # defaults to DefaultTheme
                                            # i.e. current L+F, but in jinja2 form.
  theme.route(request)

I think we can do this pretty easily by using a WSGI middleware to multiplex between the old code (django) and the new code (currently django, but this would let us switch to webapp2 at some point).

For example, Patchset should ONLY allow for the comments-related fields to change (and 'hidden'). Comments can change for viewers, but the 'hidden' field can only change at the hand of an editor.

The module-packing script currently works by reading 'deps.json' and doing the appropriate modifications to the output zip-module in-process. Should this be amended to simply symlink the third-party deps into place? It will make development on windows trickier (though not impossible), but it may make development overall easier (since the modules will actually be in the right place.)

e.g. if you hit /codereview/api/v2/issues/0 It will construct an ndb.Key('Issue', 0), which asserts the id. The key middleware should handle bad key construction and turn it into a 404.

We need a mechanism for features like:

• Linting
• Chromium TryServer Support
• Patchset reverting
• etc.

To be pluggable. This would enable the development of alternate/improved versions of these functionalities side-by-side with the current functionality, as well as affording us the ability to (finally) merge the chromium and default branches (which I consider a requirement, though I'm willing to chat about it).

Here's a feature set for the extension mechanism. It would be fine to add the bare minimum to support lint+tryserver+revert, etc, with an eye for allowing the rest later (so they can be easily added on-demand):

• Can have Settings in the user-settings page
• Can be selectable at runtime
  • Enabled by an admin
  • Have some way to decide if they apply to a given view request or not
• Can extend REST api (by adding new endpoints, probably with some namespaced schema like /api/*/ext/<ext_name>/*, where OPTIONS on /ext will tell you which extensions are available on that endpoint or something), not by modifying existing ones)
• Can extend model (by adding new Entity types (rooted on Account, Issue, or a new root), not by modifying existing models)
• Have the ability to plug in stuff to views:
  • Template context for existing views (extensions.extension_name.*)
  • Additional template formatting on Patchsets, Issues, etc.
    • This would probably need to be themeable, so an extension could have a 'default' look, but then also support the themes available on the server on a per-theme basis.
    • Sounds tricky. I'm guessing CSS won't be enough to properly style this stuff.

i.e. GET /.../drafts on a patch without drafts... should that return 200 with an empty list? Or should it return 404?

I'm thinking it should return 200 w/ empty list (unless the patch doesn't exist, of course), but I'm not sure what's correct...

ala rfc2616 as much as we can

POST - 201 if there is a body
everything else defaults to 200 if there's a return body, or 204 if the return value is empty.

Would be easy enough to treat like just-another-patch on a patchset. The tricky bit is that this implies that changing the description would create a new patchset, which is new behavior. There's a bug for this somewhere in the chromium bug tracker, but I'm too lazy to look it up at the moment.

Right now, replying to a message just posts the whole reply to the Issue.

If we're careful with how we construct the outgoing mail (probably by inserting structured markers), we could actually correlate in-line responses in the message with particular Comments on the Issue.

https://chrome.google.com/webstore/detail/rietveld-usability-toolki/nmljjlfbnbekmadhbpfpkcminoejelga?hl=en-US

This may need to be implemented as a new 'theme'.

Right now when you run tests, it results in a bunch of hidden data (mostly dev_appserver logs). I am currently accessing this data by using sqlite3 on the commandline to dump the logs to a second terminal after I run the tests:

sqlite3 /var/folders/3_/*/T/*.rietveld_tests/http_test/codereview/api/issues/comments*/logs.db 'SELECT * FROM RequestLogs; SELECT * from AppLogs'

In addition, I have a patch which I keep floating in my checkout to prevent the temp folder from the current run from being auto-deleted at the end of the tests:

diff --git a/tests_v2/main.py b/tests_v2/main.py
index 0ea0f82..9036b22 100755
--- a/tests_v2/main.py
+++ b/tests_v2/main.py
@@ -53,7 +53,7 @@ def get_tmp_dir():
   _TMP_DIR = os.environ.get('RIETVELD_TMP_DIR', None)
   if _TMP_DIR is None:
     _TMP_DIR = tempfile.mkdtemp(TMP_SUFFIX)
-    atexit.register(shutil.rmtree, _TMP_DIR, ignore_errors=True)
+    #atexit.register(shutil.rmtree, _TMP_DIR, ignore_errors=True)
     os.environ['RIETVELD_TMP_DIR'] = _TMP_DIR
   return _TMP_DIR

Right now this is at the whim of the VCS. SVN gets this right (ish), but git does it automagically (and is frequently incorrect). Dunno what hg does.

The new patchset format is general enough to correctly express "new file"/"copy"/"rename", and it would be very useful for devs to be able to express that on upload, even if their VCS doesn't really express it properly. The reason is that the diffs can end up being really silly when git gets things wrong.

Right now they're hacked into the description and parsed for various purposes (hotlinking, and for controlling the permissions of the Issue).

Right now the CQ relies on dumping status messages into the Messages for the issue. While this is convenient, it is actually pretty limiting, since the Messages area is generally used for human-to-human communication.

We either need to distinguish bot-messages from human-messages, or have CQ have some other way to track it's activity log (possibly including sending emails, but not necessarily via Messages), or something else.

The API should set Access-Control-Allow-Origin: * to make the API accessible for JavaScript clients running on different servers.

I'm not sure about the right place for adding this header to the response. Either in some middleware or in app.yaml (https://developers.google.com/appengine/docs/python/config/appconfig#http_headers).

Instance maintainers should be able to configure this setting (either in app.yaml or in a custom settings file/module).

Goals:

• Keep track of quoted sections in Comment(s) / Message(s) in order to support folding on UI.
  • Bonus: track multiple levels of nesting for cooler rendering later?
• Be able to produce Message body text via the model
  • This includes generating URLs for Comments, which have the potential to change over time.

Desire:

• I would really like to be able to keep the comment/message data stored in a semantic way, rather than as a big text blob (the current way). URLs can change, and message formatting/rendering on the page can change.
• I would also like to be able to do tighter integration with email responses (see #12). I feel like storing parsed or semi-parsed data could help.

Observations:

• Storing '> ' quoted messages is actually pretty easy. See the bottom of this bug for a toy algorithm for turning multi-nested email blob text into a cons-list style representation.
• This representation also allows easy concatenation of multiple comments, as well as embedding to get nested comments, or partial comments.

Tricky bits:

• Mixing the "strongly typed" data (like Comments) with generic text-blob data. It would be nice if Comment objects could be asked for their essential data (like their key and context_line), so that a future UI could render messages more nicely. Maybe storing as a JSON dict would work? LocalStructuredProperty with a PolyModel (does that even work? space efficiency?)

reply_reply = """
> > This is
> > some sort of
> > cool multiline
>
> New comment!

Yeah, but that's silly

>
> > comment
"""

import itertools

def parse_bucket(lines):
  ret = []
  while lines:
    bucket = []
    while lines and (not lines[0] or lines[0][0] != '>'):
      bucket.append(lines[0])
      lines.pop(0)
    if bucket:
      ret.append(bucket)
    stripped = list(
      l[1:].lstrip()
      for l in itertools.takewhile(lambda x: x.startswith('>'), lines))
    lines[:len(stripped)] = []
    sub_parse = parse_bucket(stripped)
    if sub_parse:
      ret.append(sub_parse)
  return ret


def reassemble_buckets(bucket, level=-1):
  if isinstance(bucket[0], basestring):
    prefix = ('> ' * level)
    for item in bucket:
      yield (prefix + item).rstrip() + '\n'
  else:
    for item in bucket:
      for subitem in reassemble_buckets(item, level + 1):
        yield subitem


print `reply_reply`
parsed = parse_bucket(reply_reply.splitlines())
print parsed
print `''.join(reassemble_buckets(parsed))`
print '-----'
print ''.join(reassemble_buckets(parsed + parsed))
print '-----'

With output:

"\n> > This is\n> > some sort of\n> > cool multiline\n>\n> New comment!\n\nYeah, but that's silly\n\n>\n> > comment\n"
[[''], [[['This is', 'some sort of', 'cool multiline']], ['', 'New comment!']], ['', "Yeah, but that's silly", ''], [[''], [['comment']]]]
"\n> > This is\n> > some sort of\n> > cool multiline\n>\n> New comment!\n\nYeah, but that's silly\n\n>\n> > comment\n"
-----

> > This is
> > some sort of
> > cool multiline
>
> New comment!

Yeah, but that's silly

>
> > comment

> > This is
> > some sort of
> > cool multiline
>
> New comment!

Yeah, but that's silly

>
> > comment

-----

Currently Rietveld sends email as the 'sender' of any given Message. I had planned to make Rietveld send mail via a task queue, but jrobbins@ brought up a great point, which is that appengine only allows mail transmission from a user, if that user is currently attached to the session...

We could do a couple things:

1. Try to attach the user to the taskqueue task session (probably impossible, but maybe we could just pass through cookies?)
2. Send from a central account (maybe "Useralias <[email protected]>"?)
3. Give up and send as the user non-transactionally after the message has been fully committed to the datastore.

I think I'm leaning towards 2. I don't think I would mind getting an email from '[email protected]', and that shouldn't mess up anyone's email contacts...

It would be really nice if 1. wasn't impossible/likely to break...