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

Lua Toolbox merge #86

Merged
merged 11 commits into from Sep 27, 2016

Conversation

Projects
None yet
4 participants
@Etiene
Contributor

Etiene commented Mar 17, 2016

A brand new dump of LuaToolbox's db will be needed (cc @catwell)

For transferring endorsements to Followings I added a new tab on the user settings explaining the situation with a link for the transfer. I have an issue once the transfer is done though. I'm rendering the same view from a different action and this results on the url changing when you get the success message. I don't particularly like that but I didn't know how to do otherwise and this might need a touch-up. Not a huge deal, though.

When importing current labels I was not exactly sure how to access Lapis' model module in a simple script. So for the moment the functions are in a helper class and I tested and ran them using the specs.

I also added the labels to the front page, but I'm not sure if it's pretty enough.

For the moment only module owners can add or remove labels from a module. There's no way to add new labels yet, only to select from existing labels.

Screenshots of my local system:




|--> Owner's view: Add label link is on owner's menu for the moment


@catwell

This comment has been minimized.

catwell commented Mar 17, 2016

This is great. I still miss one of the main features from Toolbox though: the ability to see the list of modules endorsed / followed by someone and a list of people endorsing / following a module. If this isn't available then followings cannot replace endorsements.

The first one could be added below the list of modules authored by a user on their user page and the second one could be accessible by clicking here:

1

Regarding DB dumps, if I give you a dump and at a later time another one, can you apply the changes? I am starting to think about exactly when I should shut Toolbox down.

@Etiene

This comment has been minimized.

Contributor

Etiene commented Mar 17, 2016

the ability to see the list of modules endorsed / followed by someone and a list of people endorsing

oops! :) coming in soon :)

@catwell

This comment has been minimized.

catwell commented Mar 17, 2016

Awesome!

I am still wondering if "follows" and "endorsements" should be the same thing. On GitHub they are not (Follow would be Watch and Endorse would be Star). What do you think?

@Etiene

This comment has been minimized.

Contributor

Etiene commented Mar 17, 2016

I had this discussion with @leafo , the issue is that maybe this makes for too many verbs and features for the users to catch up for now and the current following feature doesn't do much for the moment, he suggested merging them up and I thought it was a good idea. @hishamhm suggested maybe we should rename the followings to something simpler, such as stars, so user engagement can come easier.

@catwell

This comment has been minimized.

catwell commented Mar 17, 2016

OK, it makes sense.

Regarding missing features, I also think that:

  • Follow count should be visible in module lists (next to downloads)
  • There should be a way to sort those lists by follows (and probably downloads too)

Regarding the transition, here is how I see it:

  • Once this is ready, we decide on a date to shutdown Lua Toolbox.
  • I will send an email to all Toolbox users, telling them that it is shutting down.
    • I will explain how it will be replaced by LuaRocks (including the import feature).
    • I will tell users to create accounts there if they haven't already.
  • On the date we decided I will replace the Lua Toolbox website by a static page explaining this.
  • I will generate a final DB dump that I will give you.

How does that sound?

@hishamhm

This comment has been minimized.

Member

hishamhm commented Mar 17, 2016

Yes, my argument was that "follow" makes me think "no, I don't want to
receive email notifications", while "stars" are vague enough so that in
practice it has a community-defined meaning (see the old Twitter stars).

I also suggested that, if possible, it would be nice to import Github stars
from a project and add them up with the "local stars". This would help
populating project stars a lot.
On Mar 17, 2016 17:11, "Etiene Dalcol" notifications@github.com wrote:

I had this discussion with @leafo https://github.com/leafo , the issue
is that maybe this makes for too many verbs and features for the users to
catch up for now and the current following feature doesn't do much for the
moment, he suggested merging them up and I thought it was a good idea.
@hishamhm https://github.com/hishamhm suggested maybe we should rename
the followings to something simpler, such as stars, so user engagement can
come easier.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#86 (comment)

@Etiene

This comment has been minimized.

Contributor

Etiene commented Mar 17, 2016

@catwell sounds perfect to me!

[modules_label: "/label/modules/:label"]: capture_errors_404 =>
label = assert_error Labels\find(name: @params.label), "Invalid label"
return unless label

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

assert error jumps out of the function, so this return is unnecessary

for lm in *lmod
table.insert(query_ids,lm.module_id)
query_ids = table.concat(query_ids,",")

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

I recommend the db.list constructor for queries like this, because it guarantees the values are escaped

http://leafo.net/lapis/reference/database.html#query-interface/list

This comment has been minimized.

@Etiene

Etiene May 6, 2016

Contributor

I don't know how to create an empty paginator in case there are no results. I rewrote this and for the moment I added an if on the view

lmod = LabelsModules\select "where label_id = ?", label.id
query_ids = {0}
for lm in *lmod

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

I recommend using list comprehension, and not running the query with if there are no lmods

render: true
[remove_label: "/label/remove/:user/:module/:label_id"]: capture_errors_404 require_login respond_to {

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

any reason you decided to use user/module name slug to identify module instead of the id?

This comment has been minimized.

@Etiene

Etiene May 6, 2016

Contributor

No special reason. I was following patterns seen on other routes...

label = assert_error Labels\find(id: @params.label_id), "Invalid label id"
assert_error LabelsModules\create label_id: label.id, module_id: @module.id
redirect_to: @url_for("module", @)

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

@url_for(@module) is preferred style

name = _modules[tonumber e]
if name
m = Modules\find name: name
if m

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

oops looks like indent got goofy

class MoonRocksToolbox extends lapis.Application
[transfer_endorses: "/toolbox/transfer"]: require_login respond_to {
GET: =>

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

This should be a POST

This comment has been minimized.

@Etiene

Etiene May 6, 2016

Contributor

Ok!

if not follow
Followings\create {
source_user_id: @current_user.id
object_type: 1

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

avoid hardcoding enum values, use Followings.object_types.module

object_type: 1
object_id: m.id
}
@transfer_count = @transfer_count+1

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

moonscript has += operator

This comment has been minimized.

@Etiene

Etiene May 6, 2016

Contributor

oops

class Toolbox
create_labels_from_dump: =>
for l in *labels

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

intent got strange

for e in *endorsements
name = _modules[tonumber e]
if name
m = Modules\find name: name

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

not really critical since this is small scope, but there's the n+1 query problem here (sans the +1 though)

This comment has been minimized.

@Etiene

Etiene May 6, 2016

Contributor

I didn'nt understand this comment!

for m in *modules
mod = Modules\find name: m.name
if mod
for l in *(m.labels)

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

parens not needed

"PRIMARY KEY (id)"
}
create_table "labels_modules", {

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

I would name this table module_labels (and update the model's name as well)

import
modules
labels
from require "secrets.toolbox"

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

I don't think specs should depend on the secrets, since anyone who checks out the code will not be able to run the specs successfully. Maybe we can have a fake dump stored elsewhere

This comment has been minimized.

@Etiene

Etiene May 6, 2016

Contributor

I agree! I only added it there because I'm not sure how else to run the function I created from within helpers to import the labels from the lua dump to the sql. Should we make a separate script to do that, and then we can remove this from here or add the fake dump?

@render_modules { @module }
a href: @url_for("module", @), ->

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

@url_for(@module)

add_form: =>
@render_errors!
form action: @req.cmd_url, method: "POST", class: "form", ->

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

You can leave action blank and it will automatically post to the current page

add_form: =>
@render_errors!
form action: @req.cmd_url, method: "POST", class: "form", ->
input type: "hidden", name: "csrf_token", value: @csrf_token

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

use @csrf_input! instead

text "View Modules by Labels"
for i,l in ipairs @labels
text ", " unless i == 1
a href: "/label/modules/#{l.name}", l.name

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

use @url_for, avoid hardcoding urls

h3 "Labels"
for i,l in ipairs @labels
div class: "label_row", ->
a href: "/label/modules/#{l.name}", l.name

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

use @url_for

if can_edit
span class: "sub", ->
text " ("
a href: "/label/remove/#{@user.slug}/#{@module.name}/#{l.id}", "remove"

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

use @url_for

@@ -0,0 +1,12 @@
class Modules extends require "widgets.page"

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

Name the class after the file, so ModulesLabel. I would also rename the view/route to module_labels and ModuleLabels

@@ -0,0 +1,18 @@
class RemoveLabel require "widgets.page"

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

this is invalid syntax (probably want a spec to hit this page as well :))

@render_errors!
form action: @req.cmd_url, method: "POST", ->
input type: "hidden", name: "csrf_token", value: @csrf_token

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

@csrf_input!

input type: "hidden", name: "csrf_token", value: @csrf_token
div ->
text "Are you sure you want to remove this label from "
a href: "", @module.name

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

missing href value

input type: "submit", value: "Yes, Remove"
div ->
a href: @url_for("module", @), ->

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

@url_for(@module)

test "No endorsements were imported."
else
p ->
a href: @url_for("transfer_endorses", account), "Transfer endorsements"

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

bad reference to non-existent variable account

else
p ->
test "No endorsements were imported."

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

typo, should be text

@leafo

This comment has been minimized.

Collaborator

leafo commented Mar 18, 2016

Thanks for the patch, I did a quick code review. Tell me if you have any questions. Didn't get a chance to run the code and check out the UI yet.

I named the functionality follow because to me that says you'd like to follow the changes/updates of the module. I planned to have some sort of feature where it would give you a digest page on the site of modules you follow that got updated, and an optional email when new versions of modules you follow are released.

I think having github stars displayed could be a great way for someone browsing modules to identify what's used, but not sure about he complexities in keeping in synchronized.

}
@transfer_count = @transfer_count+1
render: "user_settings.import_toolbox"

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

Once you change this action to post, you should turn this into a redirect to go to the page that renders this view. You can pass transfer count as a query param

This comment has been minimized.

@Etiene

Etiene May 6, 2016

Contributor

I decided to move this into a helper function that returns the count

get_labels: =>
labels = {}
import LabelsModules from require "models"
lm = LabelsModules\select "where module_id = ?", @id

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

a has_many relation should exist on Modules, and then you can use the generated function here

@title = "All modules in #{label.name}"
lmod = LabelsModules\select "where label_id = ?", label.id

This comment has been minimized.

@leafo

leafo Mar 18, 2016

Collaborator

a has_many relation should be added to Labels, then you can use it here

@leafo

This comment has been minimized.

Collaborator

leafo commented Mar 21, 2016

Are you planning on making these changes @Etiene? I know your time is limited for this project, no need to rush, just curious.

@Etiene

This comment has been minimized.

Contributor

Etiene commented Mar 21, 2016

the time for the project is up, had to shift to report writing, but I will make these changes! :)

Squashed commit of the following:
commit e5ec6ff
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Fri May 6 23:03:50 2016 +0200

    Refactor

commit a69197b
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Fri May 6 22:15:07 2016 +0200

    Refactor

commit 368cd1b
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Fri May 6 22:04:16 2016 +0200

    Refactor

commit 5240e5f
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Fri May 6 21:59:43 2016 +0200

    Refactor: User post on import endorsements

commit b220a1e
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Fri May 6 01:10:01 2016 +0200

    Refactor

commit c6187dd
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Fri May 6 00:39:53 2016 +0200

    Fix indentation

commit 70f3c0e
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Fri May 6 00:34:56 2016 +0200

    Change style on url redirect on remove label

commit f7ae6d7
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Fri May 6 00:15:42 2016 +0200

    Use list comprehension on module label list

commit c384ebd
Author: Etiene Dalcol <dalcol@etiene.net>
Date:   Thu May 5 20:09:38 2016 +0200

    Remove unnecessary return
@Etiene

This comment has been minimized.

Contributor

Etiene commented May 6, 2016

Sorry for the delay! Finishing masters, moving to a different country and stuff!

@leafo I've made a refactor following most of your suggestions. I'll need help with some of them, though, I'll reply to those one by one. Could you take another look now?

@catwell I'll start working on your suggestions now

action: @url_for(@module_endorsing and "unendorse_module" or"endorse_module", module_id: @module.id)
method: "post"
}, ->
@csrf_input

This comment has been minimized.

@leafo

leafo Jul 6, 2016

Collaborator

this should be method call

text " "
span class: "header_count", "(#{@pager and @pager\total_items! or 0})"
if @pager

This comment has been minimized.

@leafo

leafo Jul 6, 2016

Collaborator

indentation goofy

@leafo leafo merged commit 4140bce into luarocks:master Sep 27, 2016

@leafo

This comment has been minimized.

Collaborator

leafo commented Sep 27, 2016

Alright! Sorry for the huge delay. I've merged in your changes. I hope you don't mind but I ended up refactoring things a bit to simplify the model structure. Labels are stored right on the module with an postgres array type. I also moved the label stuff into a new application, and added tests.

I've hidden the endorsement -> follows stuff at the moment. Once I get the data dump I can test running it, then add it back.

Thanks for your help getting this stuff done, sorry for the slow turnaround time.

@catwell could you email me that dump when you get a chance?

Thanks

@catwell

This comment has been minimized.

catwell commented Sep 27, 2016

@leafo email sent.

@Etiene

This comment has been minimized.

Contributor

Etiene commented Sep 27, 2016

No, dont worry, thank you! I'm sorry it took me ages to reply as well and I didn't have the time to go through all your suggestions! I'm really glad this is happening! 👯

@catwell

This comment has been minimized.

catwell commented Sep 29, 2016

I have seen that this is merged, awesome!

All that's missing for me now is:

  • follows count on module summaries;
  • ability to sort module lists by endorsements or by follows.

I will set Lua Toolbox read-only and point users to LuaRocks as soon as I find the time. (EDIT: done)

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