[webui] Refactoring user requests datatables #2680

ChrisBr · 2017-02-14T18:24:38Z

Moved logic from controller to RequestsDatatable class
Fix request sorting ordering by column (introduced by 509370a)
Fixes Sorting in request lists broken #2572

ChrisBr · 2017-02-14T18:24:48Z

@evanrolfe please have a look!

evanrolfe · 2017-02-15T10:03:49Z

src/api/app/models/requests_datatable.rb

@@ -0,0 +1,140 @@
+class RequestsDatatable
+  delegate :sprite_tag, :common_parts, :user_with_realname_and_icon, :fuzzy_time,
+           :project_or_package_link, :params, :link_to, :request_show_path, to: :@view


I dont understand what this is for?

These are view helper methods which I use e.g. in show_column or requester_column

evanrolfe · 2017-02-15T11:37:06Z

Nice work on this, the code is definitely not easy to work with! I have some suggestions with your class design though, the RequestsDatatable is too complicated and does not follow SOLID principles.

I would split RequestsDatatable into two classes: RequestsDatatable and RequestsDatatableRow. Also I don't think you should have any view methods like link_to, project_or_package_link inside a model because it doesn't follow the MVC pattern. Those methods should only ever be in views or helpers.

So I would use move the view methods to a json erb view file like it was done originally i.e.
https://gist.github.com/evanrolfe/ca834754a183f75986d825458ed55d21

(Warning: Code not tested.)

evanrolfe · 2017-02-15T15:28:08Z

src/api/app/models/requests_data_table.rb

@@ -0,0 +1,79 @@
+class RequestsDataTable


Can this go into a submodule i.e. BsRequest::DataTable and also: BsRequest::DataTableRow? Or something like that?

evanrolfe · 2017-02-15T15:30:30Z

src/api/app/models/requests_data_table.rb

+    @requests ||= fetch_requests
+  end
+
+  def length


does this method ever get used?

evanrolfe · 2017-02-15T15:36:31Z

src/api/app/models/requests_data_table_row.rb

+    else
+      super
+    end
+  end


IMO these two methods make the class more complicated than it needs to be.. There are simpler alternatives I think i.e.

Just call row.request.X since @request is an attr_reader

use def_delegators to delegate the desired methods to @request

Use SimpleDelegator to make this a decorator class for a request

evanrolfe · 2017-02-15T16:08:27Z

src/api/spec/models/requests_data_table_spec.rb

+    context 'sort direction' do
+      context 'defaults to sort by :created_at :desc' do
+        before do
+          @data = RequestsDataTable.new({ format: :json, length: 10, start: 0 }, user).rows


Why is format: :json a param here? It doesn't ever get used by this class does it?

evanrolfe · 2017-02-15T16:13:55Z

src/api/spec/models/requests_data_table_spec.rb

+             target_package: target_package)
+    }
+
+    context 'sort direction' do


Can you please have a look at the betterspecs guidelines for rspec because this isn't the way context is meant to be used.

Here is an example of how the spec could be written to follow the rspec guidelines (and also I've added tests for the untested public methods on this class - draw, records_total, count_requests:

https://gist.github.com/evanrolfe/eb422ca6766cd063b1f90012e9a97bf9

evanrolfe · 2017-02-15T16:15:44Z

src/api/app/controllers/webui/user_controller.rb

-    @requests = @displayed_user.send(request_method, search)
-    @requests_count = @requests.count
-    @requests = @requests.offset(params[:start].to_i).limit(params[:length].to_i).reorder(sorting_field => sorting_dir)
+    @data = RequestsDataTable.new(params, @displayed_user)


Can we call this something more descriptive? i.e. @requests_data_table?

evanrolfe · 2017-02-15T16:16:51Z

src/api/app/views/webui/user/requests.json.erb

+    ]
+    <% unless index == @data.rows.length - 1 %>
+  ,
+    <% end %>


looks like something is wrong with the indentation here?

- Moved logic from controller to RequestsDatatable class - Fix request sorting ordering by column (introduced by 509370a) Fixes openSUSE#2572

evanrolfe · 2017-02-16T12:45:39Z

LGTM (assuming the build passes)

This is a leftover from openSUSE#2680.

ChrisBr force-pushed the requests_table_refactoring branch 4 times, most recently from 57af8a8 to f6aa274 Compare February 15, 2017 08:47

mdeniz added the Frontend Things related to the OBS RoR app label Feb 15, 2017

ChrisBr force-pushed the requests_table_refactoring branch from f6aa274 to 8f8a9a6 Compare February 15, 2017 09:57

evanrolfe reviewed Feb 15, 2017

View reviewed changes

ChrisBr force-pushed the requests_table_refactoring branch from 8f8a9a6 to 8aa80f1 Compare February 15, 2017 10:25

hennevogel added the Refactoring 🏭 label Feb 15, 2017

ChrisBr force-pushed the requests_table_refactoring branch from 8aa80f1 to 8f8665c Compare February 15, 2017 15:17

evanrolfe suggested changes Feb 15, 2017

View reviewed changes

evanrolfe reviewed Feb 15, 2017

View reviewed changes

ChrisBr force-pushed the requests_table_refactoring branch 6 times, most recently from f94e544 to 4478970 Compare February 16, 2017 10:37

[webui] Refactoring user requests datatables

4f07d4c

- Moved logic from controller to RequestsDatatable class - Fix request sorting ordering by column (introduced by 509370a) Fixes openSUSE#2572

ChrisBr force-pushed the requests_table_refactoring branch from 4478970 to 4f07d4c Compare February 16, 2017 11:39

evanrolfe approved these changes Feb 16, 2017

View reviewed changes

evanrolfe merged commit 5039e3e into openSUSE:master Feb 17, 2017

ChrisBr mentioned this pull request Feb 20, 2017

Several fixes for request datatables #2675

Closed

ChrisBr added a commit to ChrisBr/open-build-service that referenced this pull request Mar 7, 2017

[webui] Use new DataTable class for user reviews

28fe83e

This is a leftover from openSUSE#2680.

ChrisBr mentioned this pull request Mar 7, 2017

[webui] Use new DataTable class for user reviews #2770

Merged

ChrisBr added a commit to ChrisBr/open-build-service that referenced this pull request Mar 9, 2017

[webui] Use new DataTable class for user reviews

5c21cd9

This is a leftover from openSUSE#2680.

bgeuken pushed a commit to bgeuken/open-build-service that referenced this pull request Mar 15, 2017

[webui] Use new DataTable class for user reviews

ee46c20

This is a leftover from openSUSE#2680.

ChrisBr deleted the requests_table_refactoring branch July 18, 2017 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webui] Refactoring user requests datatables #2680

[webui] Refactoring user requests datatables #2680

ChrisBr commented Feb 14, 2017

ChrisBr commented Feb 14, 2017

evanrolfe Feb 15, 2017

ChrisBr Feb 15, 2017

evanrolfe commented Feb 15, 2017

evanrolfe Feb 15, 2017

evanrolfe Feb 15, 2017

evanrolfe Feb 15, 2017

evanrolfe Feb 15, 2017

evanrolfe Feb 15, 2017

evanrolfe Feb 15, 2017

evanrolfe Feb 15, 2017

evanrolfe commented Feb 16, 2017

[webui] Refactoring user requests datatables #2680

[webui] Refactoring user requests datatables #2680

Conversation

ChrisBr commented Feb 14, 2017

ChrisBr commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanrolfe commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanrolfe commented Feb 16, 2017