improvements in routing and the app as a whole (WIP)

[Alwahsh]

I'm currently trying to improve routing and the app in general while writing tests in the progress.
I think this pull request might be very big if I wait till the end so I prefer to keep this one here and post updates piece by piece and take your comments while in the progress.
This is not ready to be merged... The routes file is not a one that's ready for production at all.

Please make comments on things you see problems with so that the final pull request has a code that is approved by all :)

[rohitpaulk]

@Alwahsh - Please split your changes into commits with meaningful messages so that it's easy for us to review the whole thing. :)

Example: https://github.com/glittergallery/GlitterGallery/pull/195/commits

[Alwahsh]

@rohitpaulk Alright :)

[sarupbanskota]

Looks like #256 is waiting on delayed_job. @Alwahsh did you get a chance to do the work-splitting within commits?

[Alwahsh]

@sarupbanskota delayed_job is already on master now as I put it in #240 :)

[Alwahsh]

Modified my old commit and added a new one... Please let me know if I'm going the right way or you think I should do something differently.

[Alwahsh]

Test coverage raised to 82% . We're getting there :)

[charithhewage]

Oops! I was working on a same thing :) Good work @Alwahsh.
Some of routes are pointing to same points. this is not good for SEO. So can we avoid it :)
we can use 301 redirection. this can be done from the route file itself right?

[Alwahsh]

@charithhewage yup, I'm looking at each route one by one so I still didn't get rid of every useless one... That's why I don't want this to be merged till I finish it fully.

we can use 301 redirection. this can be done from the route file itself right?

I didn't get that. Can you explain more?

[charithhewage]

@Alwahsh Please correct me if i am wrong :).
Here we have used two urls to a one single route (Eg).

get '/:username/:project/:xid/master/:image_name/update' => 'projects#update',
get '/:username/:project/master/:image_name/update' => 'projects#update'

but can we use something like this. This is a good practice to improve the SEO.

[Alwahsh]

@charithhewage I've not arrived at the routes you mentioned yet... See the routes I've done are the ones that use :user_id and :id.
Anyways, for your suggestion, I'm not sure this will work as intended... The :xid is used to allow access to private projects so I don't think making it redirect to the link without the :xid would have the effect we want and if the project is private, we'd have all links including :xid (Although currently, the owner can view them without that.) so it's not really 2 exactly similar links linking to the same page.
We don't want the private projects to be indexed by search engines anyway ;)

[sarupbanskota]

@Alwahsh the commits split up is better. could you pull from master? thanks!

[Alwahsh]

What has been done in the previous commit:

• Modified the commits route. Previously, we had links in the log going to "/commit/#{:tree_id}" and showing the state of the project at that particular commit. Now the links in the log go to "/commit/#{:commit_id}" where you see only files changed in this commit... This of course is to be extended later to include both versions of the files(The one in the current commit and the one from its parent.) This is fairly easy.
• The previous behavior that we had in "commit/#{:tree_id}" is now available in "/tree/#{:tree_id}" where you can see the state of the project at a certain tree... I added a button to the commit page that would transfer you to the corresponding tree page.
• Handled cases of wrong or invalid ids of trees and commits.
• In a previous PR, multiple file uploads were added... The issue with that is that each file would be added in a separate commit. When I upload multiple files, I'd rather see them all added in a single commit. I handled that here although I still need to move the functions from application_controller to the Project model.
• I've DELETED some methods and some routes that I thought were of no use at the moment... There are still others that I'm planning to remove soon if you agree with me... For example, there was a method to view contents of trees inside trees.... Currently there's no way that this could happen. I prefer we add it back after the local repo support.
  Since this came up, there's a little discussion we need to go through.. the routes in Github for seeing contents of a tree are like this:
  /tree/#{branch_or_tree_id}/#{subtree1}/#{subtree2}
  This is similar to what was implemented... Now this means that branches can't have length of 40 and Github has some hooks that would reject a push of a branch with name of length exactly = 40... I prefer we do it in GG this way:
  /branch/#{branch_name}/tree/#{subtree1}/#{subtree2} or better /branch/#{branch_name}/#{subtree1}/#{subtree2}

This is currently ready to be merged... i.e: The app should function fully with it included. I'm still not done with the changes though.
I have pushed my code to my demo app... You can see most of the changes through this page.

[sarupbanskota]

On Thu, Mar 19, 2015 at 11:21 AM, A notifications@github.com wrote:

What has been done in the previous commit:

• Modified the commits route. Previously, we had links in the log
  going to "/commit/#{:tree_id}" and showing the state of the project at that
  particular commit. Now the links in the log go to "/commit/#{:commit_id}"
  where you see only files changed in this commit... This of course is to be
  extended later to include both versions of the files(The one in the current
  commit and the one from its parent.) This is fairly easy.

yep, iirc rugged gives you a method for that. this would be a good place
to plug design-with-git too.

• The previous behavior that we had in "commit/#{:tree_id}" is now
  available in "/tree/#{:tree_id}" where you can see the state of the project
  at a certain tree... I added a button to the commit page that would
  transfer you to the corresponding tree page.
• Handled cases of wrong or invalid ids of trees and commits.

thanks for this one.

• In a previous PR, multiple file uploads were added... The issue with
  that is that each file would be added in a separate commit. When I upload
  multiple files, I'd rather see them all added in a single commit. I handled
  that here although I still need to move the functions from
  application_controller to the Project model.

+1

• I've DELETED some methods and some routes that I thought were of no
  use at the moment... There are still others that I'm planning to remove
  soon if you agree with me... For example, there was a method to view
  contents of trees inside trees.... Currently there's no way that this could
  happen.

That came in from #195. The idea was that I wanted to support directory
view, where you can create directories within GG projects. Now let's say
you create a project in GG locally, and replace it's underlying git
repository with a valid one that has directories - then you that method
would come to work. Of course there's no way that can happen in-app.
Removing it for now is cool, but instead of waiting for local repo support,
we could add a "create directory" button on the interface.. make sense?

• I prefer we add it back after the local repo support. Since this
  came up, there's a little discussion we need to go through.. the routes in
  Github for seeing contents of a tree are like this:
  /tree/#{branch_or_tree_id}/#{subtree1}/#{subtree2} This is similar to
  what was implemented... Now this means that branches can't have length of
  40 and Github has some hooks that would reject a push of a branch with name
  of length exactly = 40... I prefer we do it in GG this way:
  /branch/#{branch_name}/tree/#{subtree1}/#{subtree2} or better
  /branch/#{branch_name}/#{subtree1}/#{subtree2} This is currently ready
  to be merged...

yep, agreed.

• i.e: The app should function fully with it included. I'm still not
  done with the changes though. I have pushed my code to my demo app... You
  can see most of the changes through this page
  http://glittergallery-themonster.rhcloud.com/aa/myName/commits.

—
Reply to this email directly or view it on GitHub
#233 (comment)
.

[sarupbanskota]

one nit you could fix in subsequent changes: http://glittergallery-themonster.rhcloud.com/name/ranomd/commit/295896f8f231d80f898b881bdd6e9877226922cf doesn't need to go through a login. the only thing we want to login for is comments, or stuff to do with private repos, or following etc.

[Alwahsh]

but instead of waiting for local repo support, we could add a "create directory" button on the interface.. make sense?

That idea came to mind but I thought I'd keep getting more ideas as I go on and this PR would never end :) There's a lot of things in my mind for example:

this would be a good place to plug design-with-git too.

This is also what came to mind but I prefer we keep those for later if possible :)

one nit you could fix in subsequent changes: http://glittergallery-themonster.rhcloud.com/name/ranomd/commit/295896f8f231d80f898b881bdd6e9877226922cf doesn't need to go through a login. the only thing we want to login for is comments, or stuff to do with private repos, or following etc.

I didn't make any modifications to the authorization logic because I'm actually waiting for #244 because I would like to keep all the authorization logic in the abilities.rb file :)

Thanks for getting this merged... I hope to continue my work on it soon :)

[sarupbanskota]

yup, no worries, continue with the overall fixes first, just remember to
note them down, so we can revisit any new ideas later :)

Sarup

On Thu, Mar 19, 2015 at 4:20 PM, A notifications@github.com wrote:

but instead of waiting for local repo support, we could add a "create
directory" button on the interface.. make sense?

That idea came to mind but I thought I'd keep getting more ideas as I go
on and this PR would never end :) There's a lot of things in my mind for
example:

this would be a good place to plug design-with-git too.

This is also what came to mind but I prefer we keep those for later if
possible :)

one nit you could fix in subsequent changes:
http://glittergallery-themonster.rhcloud.com/name/ranomd/commit/295896f8f231d80f898b881bdd6e9877226922cf
doesn't need to go through a login. the only thing we want to login for is
comments, or stuff to do with private repos, or following etc.

I didn't make any modifications to the authorization logic because I'm
actually waiting for #244
#244 because I
would like to keep all the authorization logic in the abilities.rb file :)

Thanks for getting this merged... I hope to continue my work on it soon :)

—
Reply to this email directly or view it on GitHub
#233 (comment)
.

[Alwahsh]

@sarupbanskota sorry a quick question... For Pull requests... This functionality seems to either have been created then removed, or it was just started partially. Seems like it depends on a handle_pull_request method in the projects controller that does not exist at all. Am I missing something?