Good job! Now it's me and @elkuku who will review :) But just before that I ask you to sync your code with the main repo master code, because there were some changes done :)

Now I've synced my code with the main repo master code;)

Seems like there are some errors. Needs to fix them one by one;)
And I also need to do more comments on the changes made.
Seems like all the errors are about code styles;(

Now everything is fine;)
Please test and tell me if there is any problem.

It seems to me that filtering does not really work because of the session "state" we are still using. I think we should get rid of the state thingy and use only the request parameters.
Storing an item object to the session on a failed edit is yet another story...
@b2z and @mbabker - opinions please

Also I am not sure if we should really maintain both possibilities (AJAX and "non AJAX") for the list views. I believe we should do one or the other. I'd prefer just AJAX, with a big red warning in case you have JS disabled.

On the code style: You can download and copy a file containing some of the Joomla! standards definitions here: https://github.com/joomla/coding-standards/tree/master/IDE
This way you get informed about the coding style violations while you type ;)

The pagination thingy.... hmm... maybe we extend the class a bit to allow for a "onclick" property?
Not sure, everything is still quite "hacky" - I like the separation I did in the doctrine branch using a "paginator" object that holds only the data and a "pagination" object that renders.
Maybe we will come to this later.

Apart of this -- your work looks really promising

Sorry for going completely off topic here but since I already talked about translations.... I was wondering if you might have some friend, class mate, etc. that could help us with the Chinese translations?
Just point them to https://opentranslators.transifex.com/projects/p/jtracker/

THANKS!

I think we should get rid of the state thingy and use only the request parameters.

I would like to keep the state like on the GItHUb. This will allow us to implement "Personal filers".

Also I am not sure if we should really maintain both possibilities (AJAX and "non AJAX") for the list views.

I think we should support both. Moreover we need to support GET requests to allow passing the URL with predefined filters like on the GitHub.

It seems to me that filtering does not really work because of the session "state" we are still using. I think we should get rid of the state thingy and use only the request parameters.
Storing an item object to the session on a failed edit is yet another story...

I'm not a fan of the CMS' "stateful lists" where a lot of that code was derived from. It works with the legacy MVC code, but I just can't get on board with it if you're trying to build something where a model's state is injected by the controller instead of generated within the model (like I'm working on with the patchtester, see https://github.com/joomla-extensions/patchtester/blob/namespace/administrator/components/com_patchtester/PatchTester/Controller/DisplayController.php#L122-L178).

It'd definitely be nice to clean up how it's all handled, I just don't know yet what would be the "best" way to do it.

@mbabker

I'm not a fan of the CMS' "stateful lists"

Yes I was referring to the "stateful lists" not to states like "open" or "closed".
Maybe we can start with a base "list controller" that collects the request parameters for "listings" and injects this to a model.
That should be the same for AJAX and "non AJAX" uses.

@b2z

Moreover we need to support GET requests to allow passing the URL with predefined filters like on the GitHub.

Yes exactly. What if we use only GET requests so you always have a "valid" URL to copy&paste including the filters?

Still not sure what we would need the non AJAX stuff for :(

BTW: @mbabker the namespaced patchtester looks really cool

Hmm, I've just tested and filtering is working for me as for non-logged and as for logged in. BTW we do have model's state injected by the controller instead of generated within the model.

@elkuku can you retest and response what is wrong?

Still not sure what we would need the non AJAX stuff for :(

I thought non-AJAX stuff will allow to use GET requests, but seems that it dependents only on how we will handle this in the controller. If so then yes we can use only AJAX way.

What if we use only GET requests so you always have a "valid" URL to copy&paste including the filters?

This is quite a complex task, because we should rename all the filters for example filter-priority to priority and the sorting filter should be split in two different filters like direction and sort. But why not?

And in the controller I would like to see the combination of getting GET requests and User's state. This will allow us to implement "Personal filers".

@allenzhao I hope everything is clear for you here? :)
BTW pagination's' First button is not working ;)

I’m a little bit confused by your discussion, it is quite clear to me of AJAX only, but as for GET requests - I’m not clear of what you are referring. And as @elkuku mentioned earlier, I’m not clear which part is not working;(

Then if we all agree on making the list AJAX only, I still have some questions. Do I need to implement the methods in other models, to make them really response JSON objects or I can just declare them and leave them blank since I’ve declared an abstract method and not implemented it in all the descending models. Need your opinion and help here @elkuku @b2z

And for the First button - I will make a quick fix.

BTW, after this week’s PR, I’ve been reviewing into next steps and due to my finals at school, I can’t promise anything about next steps. And I will get things done the week after next, if I miss any thing I will try hard to catch up. Thanks again for your understanding. I will try to start implementing next feature next week - Category for issues. But before that, I want to know if we have reached an agreement on how to map the labels in GitHub with the categories we will have in JTracker. In #317 there's some discussion but things are still not clear ;(

First not working should be fixed in 29c3361.
However, it will make one useless post request with the pagination handler.
I'm trying to figure out how to drop this, but unfortunately I can't think of some methods. So I need your help here @b2z @elkuku

First not working should be fixed in 29c3361.
However, it will make one useless post request with the pagination handler.

Hmm, is this the same request made when you click on pages > 1 ?

but as for GET requests - I’m not clear of what you are referring

We mean that you can filter data using URL. For example take the GitHub:
?direction=asc&page=1&sort=created&state=open
This will filter on ascending direction, open page 1, sort by created and open state.

I also think that we should move all the state things to the new method like setModelState() to clean up prepareResponse() method.

Do I need to implement the methods in other models, to make them really response JSON objects or I can just declare them and leave them blank since I’ve declared an abstract method and not implemented it in all the descending models.

Well I do not think that you will have enough time to implement them. So if @elkuku ok with it you can declare them and leave blank at the moment.

And I will get things done the week after next, if I miss any thing I will try hard to catch up. Thanks again for your understanding.

It's not a problem. We completely understand you and wish you good luck ;)

I will try to start implementing next feature next week - Category for issues. But before that, I want to know if we have reached an agreement on how to map the labels in GitHub with the categories we will have in JTracker. In #317 there's some discussion but things are still not clear ;(

That'ts true. I think we should revise #317 involving more people from JBS. Leave it to us :)

@b2z

Hmm, is this the same request made when you click on pages > 1 ?

No it's not. Actually if you see the code, I will only add the pagination button after the first AJAX request and if the pages are more than one. But the onPageClick event listener will automatically request with /?page=1, and data actually has been retrieved before this pagination model. I don't know how to avoid this, since it's a totally useless and waste of server resource.

We mean that you can filter data using URL. For example take the GitHub:
?direction=asc&page=1&sort=created&state=open
This will filter on ascending direction, open page 1, sort by created and open state.

Oh now this is clear for me;)

I also think that we should move all the state things to the new method like setModelState() to clean up prepareResponse() method.

Yeah, now prepareResponse() method is a little bit huge, but I'm not sure where to put this setModelState() method. Should it be in the same controller with the current issues listing controller that I've implemented? If so, it should be called inside the prepareResponse() method, right?

Yeah, now prepareResponse() method is a little bit huge, but I'm not sure where to put this setModelState() method.

I would love to see code that is used for "pagination" inside a base "list controller" ?

So if @elkuku ok with it you can declare them and leave blank at the moment.

If we decide to use only AJAX, the code might get into the "standard" getListQuery() method ?

However, it will make one useless post request with the pagination handler.

With "useless" you mean requesting the data for the "next" page? Not sure how to avoid this..

I'm currently "on the road" so I can't really test the code. Apologies for my "doesn't seem to work" bug report - it's because I couldm't figure out what's going wrong...
I'm going to test again when back home.

@allenzhao I'm really sorry about the confusion... The thing is that we don't really have a fixed MVC pattern yet, maybe you can help us with some thoughts here

I would love to see code that is used for "pagination" inside a base "list controller" ?
Do you mean that something like AjaxListController extending AbstractAjaxController and put all this pagination stuff there? This sounds logical if we want to refactor all Listing controllers to Ajax.

If we decide to use only AJAX, the code might get into the "standard" getListQuery() method ?

Agree. But we told Allen in the beginning to create a new method :) So I think it is better not to create any abstract AJAX methods now, leaving only the concrete implementation in the Model. Then we could take it as the base and refactor all to AJAX.

OK I finally figured out what was not working on my side... The good news: @allenzhao's code is perfectly fine ;)
I tracked the "strange behavior" down to Joomla\Input::get() which, in my installation, is at version 1.1.2 instead of 1.1.1 and has the following change introduced: joomla-framework/input@fa1cc20 coming from this PR: joomla-framework/input#3
I'm not sure that this change is really B/C since PHP's empty() considers a bunch of values as "empty", like 0, "0" etc.

But I think we can "adjust" our code here by changing:

//FROM
$new_state = $this->input->get($request, null, $type);
//TO
$new_state = $this->input->get($request, $default, $type);

I also notice a "strange behavior" in the pagination:

Live site:

AJAX:

I think that both versions are actually buggy

Apart from that, I think it's good to merge.

There's actually a fix for the Input code already (joomla-framework/input#9) that just hasn't been pulled yet.

Ah, that's good news... please pull and tag then

Hell, @mbabker you act faster than I can think

Apart from that, I think it's good to merge.

Nope. We need to close an abstracted methods discussion first :) And what about GET requests? ;)

So @elkuku what do you think about this?

Agree. But we told Allen in the beginning to create a new method :) So I think it is better not to create any abstract AJAX methods now, leaving only the concrete implementation in the Model. Then we could take it as the base and refactor all to AJAX.

I'd say we should

1. Remove the abstract getAjaxListQuery() method from the AbstractTrackerListModel class.
2. Review the pagination. Currently it starts with our custom pagination, including non-AJAX links and only changes to AJAX pagination if filters are used. (see screenshots above)
   I think that's a bit confusing..
3. The twbsPagination added by this PR needs an entry in the credits file since it's not installed with Bower (maybe we can change this also)

And what about GET requests? ;)

I think that would exceed the time schedule that @allenzhao has for this task. Maybe the opener of #298 could add this final touch ?

Agree with @elkuku for reschedule and new plan :)

Main tasks of this week for me would be concentrating on this PR, however, I want to get started on #317 too.
As for the tasks:

Remove the abstract getAjaxListQuery() method from the AbstractTrackerListModel class.

I will get this done today;

Review the pagination. Currently it starts with our custom pagination, including non-AJAX links and only changes to AJAX pagination if filters are used. (see screenshots above)

Well for this, first the pagination style now is generated by twbs-pagination, and I'm not sure that if we are allowed to change or overwrite it to what we need. But if we make this page full ajax with the first request, all the page pagination then will be the same as the second screenshot. I'm not sure if we have to use the old pagination style. If yes, then the only way is to change the twbs-pagination thing.

And I would like to remove the old pagination thing and make all pagination, filtering AJAX, then everything will be totally fine.

The twbsPagination added by this PR needs an entry in the credits file since it's not installed with Bower (maybe we can change this also)

I think it can be installed with bower since it's in bower's registry. I've made a change via 1b105cf and added it into bower.json.

And for the GET thing, I'm think I can get this done this week. But

We mean that you can filter data using URL. For example take the GitHub:
?direction=asc&page=1&sort=created&state=open
This will filter on ascending direction, open page 1, sort by created and open state.
for this I'm a bit confused. Since all request will be done by the page, will this still be useful?

Apart from all that I've mentioned above, is there anything I'm still missing? If no, then I will do the following procedures:
1. Remove getAjaxListQuery() then make everything still working,
2. change the request methods into GET
3. remove the non - ajax part of issue listing
4. make style changes
@b2z @elkuku Any ideas?

Oh - and there's one more thing about the huge prepareResponse thing mentioned by @elkuku
I should implement the setModelState() method in order to remove the model state and pagination settings out of the prepareResponse(). So can it be in the current ajax controller?

Now after d18e20f

1. Remove getAjaxListQuery() then make everything still working

is done. I'm working on 2 now.

Your plan sounds ok. I do not see anything we can do here more.

1. Remove getAjaxListQuery() then make everything still working

So far so good. d18e20f did not break anything :)

And I would like to remove the old pagination thing and make all pagination, filtering AJAX, then everything will be totally fine.

I am for it.

I should implement the setModelState() method in order to remove the model state and pagination settings out of the prepareResponse(). So can it be in the current ajax controller?

Yes you are right. And in this method you should handle filtering (including support of GET-requests).

Seems like the change caused an error now...

This gets better every day

BTW, I think we should remove the twbs code in www/jtracker/twbs-pagination/* and update the references, since the files are now installed using Bower.

Yeah I will do that tomorrow;)

Cheers | 顺颂时祺
Allen Zhao | 赵泽涵

在 2014年6月29日，21:34，Nikolai Plath notifications@github.com 写道：

This gets better every day

BTW, I think we should remove the twbs code in www/jtracker/twbs-pagination/* and update the references, since the files are now installed using Bower.

—
Reply to this email directly or view it on GitHub.

@b2z
Now I think it needs some tests and some suggestions ;) I'm not sure what is missing

1. I think you do not need to return anything here and here just make $this->setModelState($model). Also remove blank line like this ;)

2. Currently sorting filter is broken. For sorting we use a combination of ordering and direction, because we receive the numbers as filter value. So we need to support both: numbers for POST filter and words for GET queries.

3. Please rename "status" filter to "state" and "stage" filter to "status". So everywhere in the code where these are used they should be renamed, For example here and here, Do not forget in the model and in issues.index.twig.

@b2z Please review again;)
And I will start working on #317 ;)

So regarding sorting - now it will work only for filters. But what about GET-queries? For example you can sort on GitHub like &direction=desc&sort=created or &direction=asc&sort=updated.

We currently have 4 options:
Filter option 1 - Newest, for GET-query I propose &direction=desc&sort=issue
Filter option 2 - Oldest, for GET-query I propose &direction=asc&sort=issue
Filter option 3 - Recently updated, for GET-query I propose &direction=desc&sort=updated
Filter option 4 - Least recently updated, for GET-query I propose &direction=asc&sort=updated

And do not forget DefaultContoller ;)

@b2z
I'm not sure, but now GET queries should be dealt within the DefaultController ?
See here
I thought the ajax controller won't be handling the request from outside?

I thought the ajax controller won't be handling the request from outside?

You are right. It handles only filter values. But GET-queries should be handled by DefaultController. How? The same way - through getting user state from the request, but variables should be different.

For example for state we use 1 and 0 in filters, but in GET it will be &state=closed and &state=open. The same for priority - we use 1, 2, 3, 4, 5 in filters, but in GET it should be: &priority=very-low, &priority=low, &priority=medium, &priority=urgent, &priority=critical and so on.

Do you get the idea?

Yes now, thanks so much;)

@b2z Please review this time;)
If this is good, I guess I can do the same way for state, priority, and so on.
Then what is left for this to get merged?

Looks good :) Proceed further ;)

Yes only this left I think. @elkuku?

@b2z @elkuku
I've finished ;)
This whole thing really toke me longer than I thought it would;)
Now what's left?

I think here it should be status, not stage ;)

@b2z Now it's good;)
Thanks by the way, I'm not a big fan of reformatting codes, so I will pay some attention next time
;)

Like 'needs review'?

Yeap. Statuses are translated.

But the statues in the database, are they two word or?

Some of them.

So that is because there is no sense to get them from the db, because you can not use them anyway...

Hmm..
I really can’t think of any method to do it. I tried to retrieve data with db because I considered this translation problem. What do you suggest? Shall we just move the support or add a ‘_’(underscore) instead of spaces between words?
Still they can’t use translated statuses… Now in my local vagrant db, the only two word status is no_reply.

On Jul 7, 2014, at 12:18 AM, Dmitry Rekun notifications@github.com wrote:

Like 'needs review'?

Yeap. Statuses are translated.

But the statues in the database, are they two word or?

Some of them.

So that is because there is no sense to get them from the db, because you can not use them anyway...

—
Reply to this email directly or view it on GitHub.

There is more :)

Anyway I think you should remove this method, then sync with master one more time and wait till we merge it. After that we will think how to handle the statuses through the GET-quieries ;)

OK, sure I will;)
Then mine is kind of out of date;(
Thanks so much, and next week I will start working on categories;)
On Jul 7, 2014, at 12:29 AM, Dmitry Rekun notifications@github.com wrote:

There is more :)

Anyway I think you should remove this method, then sync with master one more time and wait till we merge it. After that we will think how to handle the statuses through the GET-quieries ;)

—
Reply to this email directly or view it on GitHub.

After 92dcbd6 I think it's good to merge?

I think so... I will test one more time tomorrow and hope @elkuku also will do that.

hope @elkuku also will do that.

Hopefully he will find some time later this week....

Call to undefined method App\Tracker\Model\IssuesModel::getStatusByName() in /src/App/Tracker/Controller/DefaultController.php ;)

One more bug - here filter should be cmd, not word, because word strips - :)

Seems like everything works as expected.

Should we merge this?

@elkuku welcome back :) Yes - it's now good to merge. But we should do it later today - in case we need to quick fix something ;)

Well in the nearest time I will be online and can merge it. But we also will need @mbabker to run bower on the server, otherwise the pagination will not work.

Since I still manually trigger deployments, that's not an issue. I just have to remember to update Bower when I do it.

Ok, may the force be with us :D Merging it ^_^

Sent from my Android =;)
On Jul 13, 2014 11:11 AM, "Dmitry Rekun" notifications@github.com wrote:

Merged #372 #372.

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

Thanks so much for all the help!
@b2z @elkuku @mbabker

Good job @allenzhao ;) Now waiting for Categories :)

Deployed out to the site. Great stuff! :-)

Hmm... Strange, but there is a problem with dates. This code does not work properly:

[% opendate = new Date(item.opened_date); %]
[%= opendate.getFullYear()+'-'+opendate.getMonth()+'-'+opendate.getDate() %]

I do not understand how I missed it :( I do not remember that it was not working during testing. Some quick fix is needed :)

Ah I see. The getMonth() method returns value from 0-11. Will do this first thing in the morning;)

Cheers | 顺颂时祺
Allen Zhao | 赵泽涵

在 2014年7月14日，21:22，Dmitry Rekun notifications@github.com 写道：

Hmm... Strange, but there is a problem with dates. This code does not work properly:

[% opendate = new Date(item.opened_date); %]
[%= opendate.getFullYear()+'-'+opendate.getMonth()+'-'+opendate.getDate() %]
I do not understand how I missed it :( I do not remember that it was not working during testing. Some quick fix is needed :)

—
Reply to this email directly or view it on GitHub.

BTW about the pagination and double request - I opened an issue yesterday and author confirmed that this is a bug. So in the next release he will fix it.

Temporary solution is to comment this line https://github.com/esimakin/twbs-pagination/blob/master/jquery.twbsPagination.js#L65

@mbabker - can you apply this solution till the next release will be out (both in .min and normal versions) ?

Done

@mbabker thanks

@mbabker 1a059bb wil fix pagination.