ABTech / tracker

Carnegie Mellon Activities Board Technical Committee Tracker
abtech.org
22 stars 28 forks source link

Remove the need for "edit event" #118

Closed ghost closed 10 years ago

ghost commented 10 years ago

I have no clue how hard this would be, but it feels like with the new view we've moved away from the need from an actual "edit" button and most of it could be dropdown boxes or section-edit links without need for going to a separate page. I think this is especially relevant for:

event status event type (full/rental) published/not roles notes attachments

It's already implemented for comments and financial stuff. Thoughts?

hatkirby commented 10 years ago

So, this is actually a feature that already sort of existed in old tracker and which I spent a lot of time moving away from because it created a huge mess of everything. A lot of the old controllers, notably events and invoices, essentially ran the same code for the display pages and the editing pages, which 1) introduced a lot of unnecessary code clutter, making the code difficult to maintain, hence why I basically completely rewrote creating/editing events and invoices, and 2) introduced unnecessary visual clutter, which was most visible on the old invoice display pages, where the invoice lines table had extra columns that were only populated if you were in edit mode. It basically wasn't very sustainable from a code point of view, and was ugly from a user point of view.

This also goes against some of the fundamental conventions of Rails and REST. I do completely understand the desire to edit events in fewer clicks, but looking at information about an event and editing the event are two very separate actions. Comments, as you mentioned, is an exception because comments are created /about/ an event by people who don't necessarily have the permission to edit the event (once roles-based authorization is actually implemented). Finances is weird. I was mostly willing to make invoice editing sort of live on the display page because only exec would really see it. The invoice editing the way it is really isn't /too/ bad because it's more of editing information about or related to the event rather than actually changing the details of the event itself (although creating invoice JEs/marking them as paid on the invoices list is pushing it a little bit but I totally understand how useful it is for batch editing).

Basically, my opinion on this feature is that it is messy code-wise, creates visual clutter, creates confusion with permissions, and violates Rails and REST conventions.

tedgarb commented 10 years ago

I am going to support Kelly here. There are various issues with combining the edit and show views, both practical and principal. Seeing as most of our work, especially in events and finances, has been to bring things more in line with the conventions that underpin rails development, I am hesitant to take steps backwards, as these are the types of issues that made tracker unmaintainable in the first place. Unless you have a compelling argument for why this feature is absolutely necessary for tracker to continue to function, I am going to reject this request.

mickeyreiss commented 10 years ago

While I agree that additional complexity increases the cost of maintenance, I do not agree that this feature is extraordinarily difficult.

In addition, I would urge you to avoid making tradeoffs in functionality for purely technical reasons. The tools are here to serve us, not vice-versa.

Here's an example API that might accomplish the task:

GET /event/id - fetch information about an individual event POST /event/id - create a new event PUT /event/id - update the given event using either update or patch semantics

At its core, you should think of GET /event/id as returning the event information. The rendered page or form to edit it could just as well be their own resources:

GET /event_form - fetch an unpopulated form to edit event information GET /event_view - fetch an unpopulated template to display event information

We probably won't want both of these because it's simpler to maintain just one. Why not call it:

GET /event_view - fetch an unpopulated template to display event information an modify it inline.

It's not too hard to create in place forms that look like static data, but then, if the user has appropriate permissions, allow you to update the data inline.

Finally, you could make the decision to get rid of GET /event/id, since you'll never need raw event data. (Probably not the case—think mobile apps or zulip plugins that might grab data directly.) In this case, you might as well populate the inline form template and serve it from GET /event/id.

Another thought would be using file extensions to determine what you get:

GET /event/id.html - fetch a populated event view with inline forms GET /event/id.json - fetch event data without any presentation

By moving some logic to the client, and abstracting the notion of inline forms, this feature turns out to be simple and RESTful.

P.S. I totally agree that the old tracker implementation was unmaintainable. It broke many rails conventions, was not DRY and lacked MVC.

mmackie commented 10 years ago

I talked to Ed about this a little last night, but I think there would be some visual/ui/ux problems with a sloppy implementation of this.

My main concern would be visual clutter. Event status, organization, roles/techie are the only things that could be in a drop down box. Everything else would edit section buttons. That would add a lot of things (words or the symbols that mean drop down box) to what is currently a nice and clean/readable page. This is ugly on a computer, but makes the site a lot harder to use on a tablet/mobile. As it is, the spacing on the 'edit event' text had to be increased so that Yaghn could even click on it on his tablet.

tracker (for roles you'd end up with something that looks like this, except you'd also need a 'save' button)

My other concern would be workflow. Generally when I'm editing an event, I'm changing a lot of details at once, so it's nice for that all to be on the same screen. In this, it makes sense that the finance stuff is separate--that's generally Tim doing it, not the TiC. It would annoy me if I'd have to edit some things save, load another page, edit more things, etc. It's even worse if some things are only accessible from the main page, because the entire event editing process would become very fractured.

Ultimately, I think this ends up being a lot dependent on the user. For Tim, 90% of his activities on tracker are editing things. For him, an event page that makes it easy to edit stuff is advantageous. For the rest of us, tracker is mostly just read-only. For us, an event page that is as easy to read as possible is best.

hatkirby commented 10 years ago

I agree with pretty much everything Mackie said. This is much less of a technical problem than it is the fact that we are not willing to compromise the page that general members see. Like I said before, I'm willing to have editable finance stuff show up on the event display page because only exec can see it, and it is always editable. A workaround could be to just show a completely different page to HoT than to general members, but this just occurs to me as unnecessary.

I'd also like to point out that in the new view, editing most information about an event only takes two more clicks (click "Edit this event", and then "Submit" at the end) than inline editing would. I think this is a great improvement over the old view in terms of editing because the tabbing in the old view prevented you from editing more than one piece of information at a time. If, with inline editing, a user must click on a field in order to make it editable, we are increasing the number of clicks necessary anyway because then the user must click again to interact with the field. If a user does not have to click on a field to edit it, the view will be populated with unnecessary, ugly form fields that may accidentally be interacted with, which is especially dangerous say if you wanted the form to automatically save upon modifying a field at all in order to save one click for the Submit button.

Finally, I would also like to argue that when requested functionality works against well-established conventions, it's probably because the functionality is flawed somewhat. People spend a lot of time creating these conventions based on past experience with paradigms that work and those that don't, and while obviously not all paradigms always work in all situations, they are a good base to work off of. Having separate show/edit pages is an established paradigm with the advantages of separating two distinct actions, keeping editing away from people who don't need to edit anyway, keeping the show view clean, and preventing accidental modification of the model (in the case of automatic submission without a Submit button).

tedgarb commented 10 years ago

I am sensing a strong consensus towards not implementing this. I will leave it open for a few days, but I am very disinclined to support doing this.

ghost commented 10 years ago

I've been reading through all the comments. I definitely understand your points and the reasoning behind them, though I obviously didn't know a lot of it. I actually liked the "clutter" aspect of previous tracker, because as you noted I do the vast mahority of tracker editing. I'm not sure that i agree with the majority of event edits being of multiple aspects of an event though. Particularly the items I noted in my original request. Additionally, I think I'm biased coming from very slow internet--the last thing I want is more entire page loads. But I understand yalls sentiment and I respect that you know much more about the technical aspects and web protocol than I do.

If you want the financial section to change to be more mindful of these standards, we can talk about modifying/adding the batch pages to have a whole separate part of tracker where I can do things with minimal clicks. I think my most annoying multi-click excursion was solved by the additional functionality in invoice billing--automarking as billing pending and autocreating the JE.

You can close this. Kelly, talk to me if you want the finance section to be more mindful of the standards.

Sent from my pocket Atari On Jan 24, 2014 6:11 PM, "Edward" notifications@github.com wrote:

I am sensing a strong consensus towards not implementing this. I will leave it open for a few days, but I am very disinclined to support doing this.

— Reply to this email directly or view it on GitHubhttps://github.com/ABTech/abtt/issues/118#issuecomment-33270814 .