Edit teams.

[aandis]

References #3545

wip.

[aandis]

Alright! It's now possible to edit teams. 💃
There are a few ui improvements I have in mind which I'll do once this passes review. Will also add tests if this looks good.

[rohitpaulk]

Yay! 🍻 Screenshots please?

[aandis]

[aandis]

[aandis]

[mattbk]

Does this allow admins to edit teams? (Not sitting in front of machine I can test on.)

[rohitpaulk]

Let's also write to the events table with details about the changes - we need admins to be able to view an 'audit log'. I'm digging up the ticket where this was discussed..

[aandis]

#3545 (comment)?

[rohitpaulk]

yess, that one.

[aandis]

will do.

[aandis]

@mattbk not yet. I was under the impression admins already had a friendly way of doing so. I can add it if it's needed. Would be minor changes.

[rohitpaulk]

Wouldn't this return a 500 if someone tries to force another column in there? Let's throw a 400 instead?

[aandis]

It won't receive extra columns, at least not from the web endpoint. https://github.com/gratipay/gratipay.com/pull/3923/files#diff-8cd27f320d80ef3f1fed6dc52f5feafdR40 makes sure of that. This check is just in case this is called from somewhere else in future in which case they should catch the exception.

[rohitpaulk]

👍, makes sense

[mattbk]

I was under the impression admins already had a friendly way of doing so. I can add it if it's needed. Would be minor changes.

No, it's been happening through the database, so those of us without db access haven't been able to edit Teams. This PR solves us needing to do that for trivial things, however I think it's probably best to let admins edit anything a normal user can edit (someone can back me up or disagree on that).

Thanks for doing this, it should speed up the review process considerably and give people more ownership of their Team accounts.

[aandis]

Done @mattbk :)

Thanks for doing this

Just doing my share in making gratipay better. :)

[aandis]

todos - (mostly after review)

• write changes to events table.
• Add a back cancel button on the edit page.
• add rejected team check.
• on successful edit, disable modify btn and redirect to team homepage.
• add tests.

[chadwhitacre]

!m @aandis @mattbk @rohitpaulk

I love watching this happen! It's so encouraging to not be a bottleneck! :-)

[chadwhitacre]

One note I'll throw out there: let's be sure to log the old value in events and not just the new one. @kaguillera and I learned about this the hard way under #3912, where we were trying to reconstruct old usernames and weren't able to because we were (and are still!) logging the new value and throwing away the old.

[mattbk]

Editing team text fields seems to work for me (as admin).

Might be nice to have either a breadcrumb/menu for going back, or a cancel button if the form is automatically opened.

When I try to upload an image, I get this response:
{ "error_code": 500, "error_message_long": "Traceback (most recent call last):\n File \"/home/matt/Documents/github/gratipay.com/env/lib/python2.7/site-packages/algorithm.py\", line 288, in run\n new_state = function(**deps.as_kwargs)\n File \"/home/matt/Documents/github/gratipay.com/env/lib/python2.7/site-packages/aspen/algorithms/website.py\", line 115, in get_response_for_resource\n return {'response': resource.respond(state)}\n File \"/home/matt/Documents/github/gratipay.com/env/lib/python2.7/site-packages/aspen/http/resource.py\", line 59, in respond\n content_type, body = super(Dynamic, self).respond(accept, state)\n File \"/home/matt/Documents/github/gratipay.com/env/lib/python2.7/site-packages/aspen/simplates/__init__.py\", line 169, in respond\n exec(self.pages[1], context)\n File \"/home/matt/Documents/github/gratipay.com/www/%team/edit/edit.json.spt\", line 42, in <module>\n image = data[field].value # file upload\nAttributeError: 'unicode' object has no attribute 'value'\n", "error_message_short": "Internal Server Error" }

[aandis]

Interesting. I can't reproduce the error with any image types @mattbk. Can you paste the image you were trying to upload here?

[mattbk]

It was the first image that came up for "cat" on Google: https://www.petfinder.com/wp-content/uploads/2012/11/140272627-grooming-needs-senior-cat-632x475.jpg

It could be a local issue with my virtual Ubuntu setup. If nobody else is having problems, we can probably carry on.

[aandis]

Man, them cats I tell you. 😾 Works fine for me btw.

[aandis]

If nobody else is having problems, we can probably carry on.

Can somebody else please confirm?

[mattbk]

@rohitpaulk @whit537 Could one of you check the image upload?

I think I'm not building things correctly on either machine :-\ Just for the record, now I get the same error whether or not I try to change the image.

[aandis]

@mattbk can you apply for a new team locally from the home page and check whether you get the error? The image handling section is pretty much the same in both cases.

[rohitpaulk]

Could one of you check the image upload?

Testing..

[rohitpaulk]

I get the same error...

[rohitpaulk]

I'm noticing a few other errors too - On the homepage,

^ Clicking 'continue' does nothing, and I see an error in my console

[rohitpaulk]

I get the same error...

I'm curious to know how it is passing for you, @aandis 😄

[rohitpaulk]

I fixed #3923 (comment) in 3c191e1

[mattbk]

I'm noticing a few other errors too - On the homepage,

I have that too, thought it was a local bug. Guess I was wrong. +1

[aandis]

Alright. Tests added and rebased on master. 💃 This is ✅ from my side. :)

[mattbk]

Now I get "Bad Request" when I submit the edit form.

[mattbk]

I guess I will go through commits to check.
d9a5292 works.
5eeddee works.
b2dd1f9 works.
--> bccc241 I get "Bad Request" error.

[aandis]

oooooooooh. There's a csrf_token which gets sent. The tests don't replicate that. :/
Should be fixed now @mattbk

[mattbk]

Works!

The workflow to remove an image is a little weird to me (choose file->cancel), but that can probably be a different PR since most people will be replacing the image, not removing it.

Running tests locally, just to be sure. NM, if they run on Travis, I'm sold.

[aandis]

Works!

Ha! I just recently started writing tests after @rohitpaulk's continuous advice and started thinking of them as a source of truth. Lesson learnt => Never trust tests 100%. Especially your own tests. 😉

The workflow to remove an image is a little weird to me

You can't really remove a team image since you always need to have one so you'll always be replacing it. If you mean removing it from the form then maybe yeah.

[rohitpaulk]

Never trust tests 100%. Especially your own tests.

@aandis - There are tests that you can trust 100% :) Full-blown integration tests, which actually open a browser and click through, for example.

[mattbk]

You can't really remove a team image since you always need to have one so you'll always be replacing it.

You're right; I didn't click Modify to see that the image doesn't disappear.

[aandis]

There are tests that you can trust 100% :) Full-blown integration tests, which actually open a browser and click through, for example.

Cool! I'll look into the js tests and selenium someday as well. :)

[mattbk]

• @aandis's checklist is complete.
• looks like @rohitpaulk's comments have been answered (or reticketed to Make new team application and edit to work even without js #3930)
• works on my Mac
• works on my virtual Ubuntu
• passes Travis.
• Confirmed this works (from above): "One note I'll throw out there: let's be sure to log the old value in events and not just the new one."

Merging.

[aandis]

Merging.

Yay! :)

Confirmed this works (from above): "One note I'll throw out there: let's be sure to log the old value in events and not just the new one."

One thing I want to point out here before this is deployed is that I decided to only log the old changed values since the most recent values are already present in the teams table, so there's no point logging the new ones. This should be enough to generate the series of edits done for a team. I hope @rohitpaulk reviewed this already. I don't fully understand the use case of this yet, so if logging the entire tuple of team which is edited is more useful to the use case, I can change this.

Change is here and the tests for that here

[chadwhitacre]

I decided to only log the old changed values

👍 from me.

Thanks for this @aandis, awesome to see forward progress! 💃

[aandis]

💃💃