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

Filter tree based on Pull Request (Github only) #428

Merged
merged 18 commits into from Aug 29, 2017

Conversation

Projects
None yet
7 participants
@bigredwill
Copy link
Contributor

bigredwill commented Aug 23, 2017

Description

This PR addresses #313. These changes are a fork of @jefftougas' fork, which was submitted as PR #397.

This PR is different in that it uses the Github API to retrieve the PR diff from the files changed endpoint: https://developer.github.com/v3/pulls/#list-pull-requests-files

It also adds an option to the settings for Show only changed files in a pull request.

Proposal

  • On PR file list / diff tab in PRs, filter the tree to only show files / folders that have a diff in the PR
  • Diff stats are shown next to each folder / file in the tree
  • Only filter tree if option for Show only changed files in a pull request is true in settings.
  • In PR view mode, clicking any file in the tree takes you to that file's diff within the PR
    • NOTE: This was not completed. Clicking a file has the same result as clicking a file in the non filtered view. As far as I can tell, there is no way to retrieve a direct link to a file on the Files Changed page in a PR. It is possible in the other PR because the page itself is being parsed. If anyone knows a way to retrieve this without parsing the DOM on the Files Changed page, please let me know.

Settings Screen

screen shot 2017-08-22 at 5 18 28 pm

Unexpanded Filtered Tree

screen shot 2017-08-22 at 3 29 30 pm

Expanded Filtered Tree

screen shot 2017-08-22 at 3 30 04 pm

@bigredwill bigredwill referenced this pull request Aug 23, 2017

Closed

Filter tree based on PR #313

@jeremyalan
Copy link

jeremyalan left a comment

A few small requests, but it looks good overall, can't wait to see this get merged! 😄

}

if (item.patch.changes) {
patch_html += `<span>${item.patch.changes} changes</span>`

This comment has been minimized.

@jeremyalan

jeremyalan Aug 23, 2017

I would consider changing the text to # files instead of # changes -- it's a bit misleading as-is, as I would expect changes to total the number of lines that were modified.

if (typeof diffMap[path] === 'object') {
diffMap[path].additions += file.additions
diffMap[path].deletions += file.deletions
diffMap[path].changes++

This comment has been minimized.

@jeremyalan

jeremyalan Aug 23, 2017

Related to my previous comment, consider changing this property to files or similar.

diffMap[path] = {
additions: file.additions,
deletions: file.deletions,
changes: 1,

This comment has been minimized.

@jeremyalan

jeremyalan Aug 23, 2017

Related to my previous comment, consider changing this property to files or similar.

@bigredwill

This comment has been minimized.

Copy link
Contributor

bigredwill commented Aug 23, 2017

@jeremyalan done! This has been too long in the pipeline, I can't wait either 👍

@bigredwill

This comment has been minimized.

Copy link
Contributor

bigredwill commented Aug 23, 2017

Updated UI, files instead of changes

screen shot 2017-08-22 at 9 20 26 pm

@buunguyen

This comment has been minimized.

Copy link
Collaborator

buunguyen commented Aug 23, 2017

@crashbell would love to hear what you think about this PR.

@jchannon

This comment has been minimized.

Copy link

jchannon commented Aug 23, 2017

This is awesome!!🎉🎉

@buunguyen

This comment has been minimized.

Copy link
Collaborator

buunguyen commented Aug 23, 2017

@bigredwill great stuff, thanks for working on this.

I've tried it out and here are some issues:

  1. It doesn't work when the option "Load entire tree at once" is unchecked (i.e. lazy loading mode). I'm not sure if it's feasible to make this feature works with lazy loading mode; if not, at least we should probably disable this option unless "Load entire tree at once" is enabled.

  2. (minor thing) The verbiage of "Show only changed files..." is a bit long, it wraps for default sidebar width. Suggest rephrase to simply "Show only changes in pull requests".

  3. When clicking on a file in the PR view, I think it's better that it links to the PR diff, like https://github.com/buunguyen/octotree/pull/428/files#diff-ab93c06aa4a7b7156444a9205bb16c21. For now, clicking will jump to the code page, which has 2 undesirable effects: the file doesn't contain the changes made by the PR and Octotree reloads and shows the full tree instead of the PR tree.

What do you think?

@buunguyen buunguyen referenced this pull request Aug 23, 2017

Closed

Filter tree based on PR #397

@timglabisch

This comment has been minimized.

Copy link

timglabisch commented Aug 23, 2017

thanks, awesome.

When clicking on a file in the PR view, I think it's better that it links to the PR diff,

👍 👍 👍

@bigredwill

This comment has been minimized.

Copy link
Contributor

bigredwill commented Aug 23, 2017

Re @buunguyen :

  1. I'll look in to this, either making it work with lazy loading mode or disabling if not feasible.
  2. Will update.
  3. This is an issue in that the hash/id appended to the url to link to a specific file is not actually the sha of the file diff. I'm not sure where that hash/id is generated, and it is not available through any Github API as far as I can tell.
    That said, this is doable. I inspected the DOM structure of the Files Changed tab, and each file has another id associated with it that is a 0 indexed number and assigned in alphabetical order. So the first file in that tab is #diff-0, the second is #diff-1, etc. To allow for jumping to the Files Changed tab in the correct location, I'll add an diffId field to the file in _getPatch that allows us to generate a correct href.
@timglabisch

This comment has been minimized.

Copy link

timglabisch commented Aug 23, 2017

@bigredwill why not just jumping to the element with the data-path?

data_path

@bigredwill

This comment has been minimized.

Copy link
Contributor

bigredwill commented Aug 23, 2017

@timglabisch I don't believe you can use the url to scroll to an element based on a data- attribute.

The hash/id appended to the url tells the browser to jump to the first element on the page with that hash/id as it's id or name attribute.

See the image below. The div container for src/adapters/bitbucket.js contains the attribute id="diff-1". This is because it is the second file changed on the page. Linking to this will work.

screen shot 2017-08-23 at 2 02 17 pm

@bigredwill bigredwill force-pushed the bigredwill:pr_diff_tree_api branch from f19f902 to 7a611a6 Aug 23, 2017

@bigredwill

This comment has been minimized.

Copy link
Contributor

bigredwill commented Aug 23, 2017

@buunguyen These three recent commits should take care of all the issues you presented :)

Works with lazy loading as well 🎉

}
} else {
// encodes but retains the slashes, see #274
const encodedPath = path.split('/').map(encodeURIComponent).join('/')

This comment has been minimized.

@buunguyen

buunguyen Aug 24, 2017

Collaborator

Lol, thanks to your PR, I notice there's a bug in the original code :), encodedPath should be used in line 115 below. I think line 109 should be encoded the same way.

This comment has been minimized.

@buunguyen

buunguyen Aug 24, 2017

Collaborator

In this PR: babel/babel#5743, the tree of the first child of packages can't be loaded. When lazy mode is active, it will load the trees but not the nodes/files. Please take a look.

Updated: I've seen this behavior in a few PRs already (others are in private repos so can't post link).

This comment has been minimized.

@bigredwill

bigredwill Aug 24, 2017

Contributor

I noticed the encodedPath thing as well, want me to update that with this PR?

This comment has been minimized.

@buunguyen

buunguyen Aug 24, 2017

Collaborator

Yes, please. I think you'll need to change the case for patch too, if username/repo has forbidden characters which need encoding.

.filter((node) => {
// If lazy load, prepend the path of node that's currently loading
let nodePath = opts.node.path ? `${opts.node.path}/${node.path}` : node.path
return patchRes[nodePath] !== undefined

This comment has been minimized.

@buunguyen

buunguyen Aug 24, 2017

Collaborator

We probably don't need the map call by adding:

node.patch = patchRes[nodePath]
return !!node.patch
let patch_html = ''

if (item.patch.action) {
if (item.patch.action === 'add') {

This comment has been minimized.

@crashbell

crashbell Aug 24, 2017

Contributor

I think a switch...case statement would be clearer and more extendable.

This comment has been minimized.

@buunguyen

buunguyen Aug 24, 2017

Collaborator

Also, I don't think the outer if (item.patch.action) is necessary.

const reponame = match[2]
let username = match[1]
let reponame = match[2]
let type = match[3]

This comment has been minimized.

@crashbell

crashbell Aug 24, 2017

Contributor

Should we add typeId here?

This comment has been minimized.

@bigredwill

bigredwill Aug 24, 2017

Contributor

I think that makes sense


// Not a repository, skip
if (~GH_RESERVED_USER_NAMES.indexOf(username) ||
~GH_RESERVED_REPO_NAMES.indexOf(reponame)) {
return cb()
}

// Check if this is a PR and whether we should show changes
const isPR = type === 'pull'
const pull = isPR && showOnlyChangedInPR ? match[4] : null

This comment has been minimized.

@crashbell

crashbell Aug 24, 2017

Contributor

pullRequestId or pullId or pullNumber would be clearer.

if (patchErr || !diffExists) cb(null, res.tree)
else {
// Filter tree to only include files and directories that are included in the patch
const filteredTree = res.tree

This comment has been minimized.

@crashbell

crashbell Aug 24, 2017

Contributor

A reduce function can make it more simple.

const filteredTree = res.tree.reduce((result, node) => {
  const nodePath = opts.node.path ? `${opts.node.path}/${node.path}` : node.path
  if (patchRes[nodePath]) {
    node.patch = patchRes[nodePath]
    result.push(node)
  }
}, [])

This comment has been minimized.

@bigredwill

bigredwill Aug 24, 2017

Contributor

Another bug I just caught with this code:
If a file is added or renamed, it doesn't exist in res.tree. This means these files won't show up in the tree view.

To fix this, we will only use the result from _getPatch and return an array with objects that are similarly structured to those returned by the /git/trees/${path} call. This has the added benefit of only making one network call rather than two as it is now.

This comment has been minimized.

@buunguyen

buunguyen Aug 24, 2017

Collaborator

@bigredwill sounds good. In that case, I suppose when PR mode is used, the lazy mode is not used. I think that's a plus, avoid one source of bugs and it isn't necessary to lazy load PR changes as lazy loading only makes sense for huge repo, not a single change set.

const filteredTree = res.tree
.filter((node) => {
// If lazy load, prepend the path of node that's currently loading
let nodePath = opts.node.path ? `${opts.node.path}/${node.path}` : node.path

This comment has been minimized.

@crashbell

crashbell Aug 24, 2017

Contributor

I don't see we have a null-check here for opts.node. Can it cause an error?

path = `${curr}`
}
// Path already has been recorded, accumulate changes
if (typeof diffMap[path] === 'object') {

This comment has been minimized.

@crashbell

crashbell Aug 24, 2017

Contributor

I think we can make it shorter with if (diffMap[path]).

patch_html += `<span>${item.patch.filesChanged} ${fileString}</span>`
}

if (item.patch.additions !== 0 || item.patch.deletions !== 0) {

This comment has been minimized.

@crashbell

crashbell Aug 24, 2017

Contributor

For zero number, I think it makes more sense to have it empty. +0 or -0 is not informative to me.

item.a_attr = {
href: this._getItemHref(repo, type, path)
// If item is part of a PR, jump to that file's diff
if (item.patch && typeof item.patch.diffId === 'number') {

This comment has been minimized.

@crashbell

crashbell Aug 24, 2017

Contributor

typeof item.patch.diffId === 'number' => I'm a bit curious whether there're other data types.

This comment has been minimized.

@bigredwill

bigredwill Aug 24, 2017

Contributor

Initially I was checking if (item.patch.diffId), but that returns false when diffId === 0

@crashbell

This comment has been minimized.

Copy link
Contributor

crashbell commented Aug 24, 2017

Thanks @bigredwill I have added some comments.

@buunguyen

This comment has been minimized.

Copy link
Collaborator

buunguyen commented Aug 28, 2017

@bigredwill thanks for the update.

I've just noticed file download doesn't work in PR mode:

  • The file name is wrong (there's the "added", "renamed" suffix).
  • The content is not the file itself, but the entire HTML page to the diff link.

Can you take a look?

Other than that, seems great, I'm very keen on merging this PR.

@bigredwill

This comment has been minimized.

Copy link
Contributor

bigredwill commented Aug 29, 2017

@buunguyen still working on making this work and cleaning up the code. Also updating per @crashbell 's suggestions.

About the file download. Do we want to download the file diff? Or the full file itself?
See what's available in https://developer.github.com/v3/pulls/#list-pull-requests-files

@buunguyen

This comment has been minimized.

Copy link
Collaborator

buunguyen commented Aug 29, 2017

I think it should download the entire file (with the changes). If people want to see the diff, the can already click and see it here.

@bigredwill

This comment has been minimized.

Copy link
Contributor

bigredwill commented Aug 29, 2017

@buunguyen These latest changes should fix the downloading.

I had to add data-downloadHref and data-downloadFileName attributes so that we could access these from within the click handler. Because the link in the PR view is not the actual file blob page but to the diff on the Files Changed page, these two attributes are needed. The downloadHref attribute gives us the link to the blob, and the downloadFileName attribute gives us the name of the file (rather than rely on the anchor element's inner text).

this._get(`/git/trees/${path}`, opts, (err, res) => {
if (err) cb(err)
else {
console.log(res.tree)

This comment has been minimized.

@crashbell

crashbell Aug 29, 2017

Contributor

Is this redundant?

This comment has been minimized.

@bigredwill

bigredwill Aug 29, 2017

Contributor

Yup, was debugging, need to remove this..

// Iterate files/folders to determine diff details
res.forEach((file, index) => {
// Grab parent folder path
const folderPath = file.filename.split('/').slice(0,-1).join('/')

This comment has been minimized.

@crashbell

crashbell Aug 29, 2017

Contributor

Please add a space between arguments: slice(0, -1)

})
})
// Sort by path, needs to be alphabetical order (so parent folders come before children)
diffTree.sort((a,b) => a.path.localeCompare(b.path))

This comment has been minimized.

@crashbell

crashbell Aug 29, 2017

Contributor

(a,b) => (a, b)

@bigredwill

This comment has been minimized.

Copy link
Contributor

bigredwill commented Aug 29, 2017

@crashbell most recent commits clean up code.
@buunguyen
I fixed the case for removed patch status. It looks like this now:
screen shot 2017-08-29 at 11 12 55 am

@buunguyen

This comment has been minimized.

Copy link
Collaborator

buunguyen commented Aug 29, 2017

@bigredwill awesome, thanks!

@buunguyen buunguyen merged commit f9bb5b7 into ovity:master Aug 29, 2017

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