Skip to content
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

Add new project from admin #52

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Bounga
Copy link
Contributor

@Bounga Bounga commented Mar 28, 2018

Add form for adding a new project and spawn a sidekiq worker to import all new contributors and commits for the given project.

@Bounga
Copy link
Contributor Author

Bounga commented Mar 28, 2018

It is related to #39 /cc @davydovanton

@Bounga
Copy link
Contributor Author

Bounga commented Mar 28, 2018

I think we can refactor internals of service methods

@davydovanton
Copy link
Member

oh, sorry, I missed this PR. Will check it ASAP. Sorry again 😞

@Bounga
Copy link
Contributor Author

Bounga commented Mar 31, 2018

@davydovanton no problem. Check it when you can.

@Bounga
Copy link
Contributor Author

Bounga commented Mar 31, 2018

You don’t have to apologize. Really.

Copy link
Member

@davydovanton davydovanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good for me. I'll play with it locally and after that merge. Thanks for your work!

@davydovanton
Copy link
Member

also, could you fix sidekiq specs? Just use Sidekiq::Testing.fake! for this and check how much jobs was spawned
https://github.com/mperham/sidekiq/wiki/Testing

@Bounga
Copy link
Contributor Author

Bounga commented Apr 2, 2018

@davydovanton Do you understand the Ruby version related error? I don't experience this locally. Other than that I have one error left:

 1) Admin::Views::ApplicationLayout contains application name
     Failure/Error: let(:rendered) { layout.render }
     
     NoMethodError:
       undefined method `view' for #<Hanami::View::Template:0x00007fae1dff26e8>
     # ./vendor/bundle/gems/hanami-view-1.1.0/lib/hanami/view/rendering/layout_scope.rb:119:in `view'
     # ./vendor/bundle/gems/hanami-view-1.1.0/lib/hanami/view/rendering/layout_scope.rb:248:in `renderer'
     # ./vendor/bundle/gems/hanami-view-1.1.0/lib/hanami/view/rendering/layout_scope.rb:101:in `render'
     # ./apps/admin/templates/application.html.slim:10:in `block in singleton class'
     # ./apps/admin/templates/application.html.slim:-2:in `instance_eval'
     # ./apps/admin/templates/application.html.slim:-2:in `singleton class'
     # ./apps/admin/templates/application.html.slim:-5:in `__tilt_70192868882380'
     # ./vendor/bundle/gems/tilt-2.0.8/lib/tilt/template.rb:170:in `call'
     # ./vendor/bundle/gems/tilt-2.0.8/lib/tilt/template.rb:170:in `evaluate'
     # ./vendor/bundle/gems/tilt-2.0.8/lib/tilt/template.rb:109:in `render'
     # ./vendor/bundle/gems/hanami-view-1.1.0/lib/hanami/view/template.rb:41:in `render'
     # ./vendor/bundle/gems/hanami-view-1.1.0/lib/hanami/layout.rb:139:in `render'
     # ./spec/admin/views/application_layout_spec.rb:5:in `block (2 levels) in <top (required)>'
     # ./spec/admin/views/application_layout_spec.rb:9:in `block (2 levels) in <top (required)>'

and I can't understand why either.

@davydovanton
Copy link
Member

hey, sorry for the long delay. I think the issue in this line:

= render partial: 'shared/navbar'

Also, you can try to remove (or comment this spec: https://github.com/hanami/contributors/blob/master/spec/admin/views/application_layout_spec.rb#L9)

@Bounga
Copy link
Contributor Author

Bounga commented Jul 16, 2018

Wow what the xitkeyword visible here https://github.com/hanami/contributors/blob/master/spec/admin/views/application_layout_spec.rb#L8 ?

Don't have this locally, it's itas it should be…

@Bounga
Copy link
Contributor Author

Bounga commented Jul 16, 2018

Seems like it was introduced later

Ensure that when a project is added in admin it spawns a new
background job to add contributors and their commits for the given
project.
@Bounga Bounga force-pushed the add-new-project-from-admin branch from b7117a8 to 1a89143 Compare July 16, 2018 14:06
@Bounga
Copy link
Contributor Author

Bounga commented Jul 16, 2018

@davydovanton Here we are. Everything seems fine now.

@jodosha jodosha changed the base branch from master to main June 17, 2021 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants