Closed Bug 1306494 Opened 9 years ago Closed 5 years ago

Add a diff+commit submit button to some text areas in tc-tools

Categories

(Taskcluster :: UI, enhancement)

Product:
Taskcluster
Component:
UI
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bstack, Assigned: ss77995ss, Mentored)

Details

Long-term I'd like it if certain sorts of fields we configure (such as scopes assigned to a client) were versioned with a nice history and blames, etc. However, that's a big change, so for the time being, when I go to change a big scary list of strings (like the scopes for an important service or the team_taskcluster scopes, I'd like it if the button to save changes popped up an interstitial that showed a nice little diff of my changes and "commit" or "cancel" buttons. This would give me greater confidence that the changes I just made are correct and that I didn't accidentally delete a line or something. This could maybe be an optional attribute of all of our text areas. I don't want it on every field (like descriptions or whatever) but any that are kinda important.
Note that scopes aren't a text blob in the API -- they are actually a list of strings. So someone smarter than me could probably make a UI that represents this without an interstitial. For other sorts of edits, though, I agree -- workerTypes, especially, are scary this way.
Hi, this bug is interesting. May I work on this? (And may I have more information w.r.t the source code related to the bug?)
Sure! Welcome! The source for the tools site (https://tools.taskcluster.net) is at https://github.com/taskcluster/taskcluster-tools
Assignee: nobody → ghosalatreyee
Thanks for looking into this, atreya! The code in particular where I think this would be best to add first is in https://github.com/taskcluster/taskcluster-tools/tree/master/src/components/ScopeEditor. When you run the local development server this code is used on this page: http://localhost:9000/auth/clients/
Atreya, how is this going?
Keywords: good-first-bug
If no one is working on this, can I please take this up?
Flags: needinfo?(dustin)
You may! I cleared the "good first bug" flag from this, as it needs some thought before implementing. Please have a look at the tools site and how things work now, then think about how you might change them, then describe your ideas here so that we can discuss them, before implementing.
Assignee: ghosalatreyee → mynah.marie
Flags: needinfo?(dustin)
Ok great, thank you! Will update with my ideas :)
So I had a look at the source code at the specific place mentioned by Brian and I set up my dev environment to start working on this. I'm starting to have a few ideas on how it would be possible to implement it: Design: As to not mess too much with the layout, I was thinking to maybe display the scopes to review in the same space where the scopes are listed (so basically something that would look very similar to the CodeEditor except for maybe colour highlights depending on the changes the user made...). I have a question regarding the buttons, right now if a user is editing there are 3 buttons displayed including a "Save" button. Do we want to change the "Save" to "Review" and then display the Save button while on the reviewing stage? Or should we add a 4th button for Review and give the option to the user to save right away? Not sure which would be best or if there is a better option... Logic: For the logic implementation I wrote a bit of pseudo code to try and find a way to implement this, so far this is what I came up with: // In the constructor, add another piece of state and set it first to the original state of the scopes. constructor(props) { super(props); this.state = { scopeText: props.scopes.join('\n'), oldScopes: props.scopes, scopesDiff: {} }; } // Add a renderDiff function that will act as intermediary between the View and Save. renderDiff = (oldScopes, newScopes) => { //Create 3 arrays, 1 for each possible kind of scope (no change, added, deleted) let addedScopes = []; let deletedScopes = []; let sameScopes = []; //Create an object that will contain all 3 arrays. let scopesDiff = {}; //Define an object with our old scopes with a key/value pair that will be set depending on whether we checked //if this scope is part of the new list or not. let oldScopesChecked = props.scopes.reduce((obj, item) => { obj[item] = false; return obj; }, {}); newScopes.forEach(scope => { if (oldScopes.includes(scope)) { // If this scope is present in both arrays, no changes to display. sameScopes.push(scope); oldScopesChecked[scope] = true; } else { // Otherwise it has been added. addedScopes.push(scope); } }); //Check if there are any scopes in the old scopes that are still set to false. If yes, // it means they have been deleted. deletedScopes = Object.keys(oldScopes).map(scope => { return oldScopesChecked[scope] ? null : scope; }).filter(item => { return item !== null; }); //Set our scopesDiff object in the state, which we will map through for rendering. scopesDiff = {sameScopes, addedScopes, deletedScopes}; this.setState({ scopesDiff }); } // In the render, add a third view option for the diff. render() { if (this.props.review) { // map through each array contained in this.state.scopesDiff and adjust the css accordingly } else if (this.props.editing) { return this.renderScopeEditor() } else { return this.renderScopes(); } } I have a feeling this logic might be a bit simplistic... For example it doesn't take into account the original indexing of each element. I guess it depends on the format we'd like to have for displaying the changes... Would it be better to have something like: Added Scopes: ...(display all the scopes added by the user (in green?) )... Deleted Scopes: ...(display all the scopes added by the user (in red?) )... (in this case should we leave out the unchanged scopes?) Or are we aiming for something that is closer to what we see on github in PRs with the whole file and the deleted parts highlighted red, added parts highlighted in green, unchanged parts in normal black letters... Looking forward to hear thoughts, advice and feedback on this :)
> Do we want to change the "Save" to "Review" and then display the Save button while on the reviewing stage? Or should we add a 4th button for Review I was thinking about this today too, and I think we should add another button for toggling between "Diff" and "Editor". Not sure what the best naming is for this yet, but feel free to propose something nice. > I have a feeling this logic might be a bit simplistic... For example it doesn't take into account the original indexing of each element. I guess it depends on the format we'd like to have for displaying the changes I think the easiest way to perform the diff, and reduce a lot of logic, is to sort the list lexicographically before rendering the editor, and before showing the diff. As long as we always sort the list prior to being placed in state, I think it would make it easy to switch between the views and show the diff. > Or are we aiming for something that is closer to what we see on github in PRs with the whole file and the deleted parts highlighted red, added parts highlighted in green, unchanged parts in normal black letters... We can still achieve something like GitHub does, just have to think about how this would work with sorting. https://www.npmjs.com/package/diff
Great analysis, thanks Mynah! Regarding diff/editor -- that sounds similar to Github's edit/preview tabs. I agree regarding sorting the scopes. Most scopes have a least-specific-to-most-specific naming, so sorting them makes sense (e.g., it puts all of the `queue` scopes together, then all of the `queue:create-task:` and so on.) I know brian disagrees with me :)
Hi! Sorry I took some time to reply, I was finalizing my Outreachy application :) Thank you Eli and Dustin for the valuable feedback! > I think we should add another button for toggling between "Diff" and "Editor". Not sure what the best naming is for this yet, but feel free to propose something nice. I will think about that... Right now I don't have any amazing idea on how to name it but I will keep in mind that the direction is to add a 4th button with toggling functionality. Maybe we can discuss further about the naming while in the implementation process... > Regarding diff/editor -- that sounds similar to Github's edit/preview tabs. That's something I didn't consider but maybe that could also be an option, to have the "diff" in another tab rather then click on a button to view it? > I think the easiest way to perform the diff, and reduce a lot of logic, is to sort the list lexicographically before rendering the editor, and before showing the diff. Cool, yes sorting the list seems like it would definitely make it easier to implement. Also, Eli thank you for pointing me to the diff npm package! Is there anything else we should discuss before I start working on the implementation?
It sounds like you have a good idea what you want to do and are ready to get started :)
Ok I've started to work on this, will post here if I have any more questions :)
@dustin @Eli I've created a first PR for this issue, I really don't think it's the final solution but I found this module for React (called `react-diff`) that uses the `diff` module under the hood so I thought that maybe it would be good to make a PR with a basic implementation so that you see where I'm heading and if you think this could be a good starting point. What I'm thinking of doing is customizing this `react-diff` module so that I can change the styling and also configure the way it uses the `diff` library (there are some functions in the `diff` module that I think might be better for what we are trying to achieve... I tried working directly with the `diff` module by importing it but somehow couldn't make it work...). I already started to work on a customized version but I thought maybe it's better I first push a basic version that uses the original module so that you can tell me if you think this might be the right direction :) Also, I'm still wondering about the styling... Right now the new button is also green but should probably be a different colour and I stuck with "Review" and "Edit" for now until maybe something better pops up... :D
@dustin @Eli I've changed my initial PR to add a customized version of the 'react-diff' module already because I realized I didn't need to make much changes to it in order to have it behave in a way that I think is closer to what the idea is (in terms of styling but also for displaying the changes as lines instead of each individual word). I also changed the colours of the buttons in the toolbar to something that might be more appropriate... Curious to see what you think of those changes, if you think this is not the right direction I can always try other angles :)
See https://github.com/taskcluster/taskcluster-tools/pull/513
Assignee: mynah.marie → nobody
Severity: normal → enhancement
Component: Tools → UI and Tools

This is still valid but should be done in the new UI (not tc-tools).

Hi, is this bug still available? I'm interested in working on this one, thanks!

Assignee: nobody → ss77995ss

I think I agree with the approach in https://bugzilla.mozilla.org/show_bug.cgi?id=1306494#c10. We actually already do something similar in https://taskcluster-ui.herokuapp.com/hooks/garbage/abc456 where we have a "WRITE" and "PREVIEW" tab for markdown text. We could use something similar for displaying the diff.

Hi, I created a PR, please check, thanks!

I'm not sure where to put this function, so I just add it at Hook page to demo.

Also, I didn't change too much of style because I do not have any ideas yet, let know if you have any ideas, thanks!

There is a PR that's almost ready to be merged. Before we do, would it make more sense to show the expanded scopes diff rather than the scopes diff after making changes to the list of scopes?

Flags: needinfo?(bstack)

Good question. I think limiting it to exactly what you're editing makes sense for now? Maybe we could show both for scopes diffs?

Honestly I think let's do whichever is easier for now and we can iterate on it later if we want to improve! Nice work to both of you!

Flags: needinfo?(bstack)

Completed in https://github.com/taskcluster/taskcluster/commit/d999bf75e74ebe8db4b1872b61a6dcfb8aa03e9d. A big thank you to Sean for their hard work \o/

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

How cool is that!

You need to log in before you can comment on or make changes to this bug.