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

feat: Add CFS filter to sidebar filters in explore route. #3144

Merged
merged 10 commits into from Jul 6, 2019

Conversation

@Anupam-dagar
Copy link
Member

Anupam-dagar commented Jun 14, 2019

Reference: #3098

Short description of what this resolves:

This adds a new search filter to sidebar of explore route.

Changes proposed in this pull request:

  • Add a new filter accordion with open and closed options in sidebar.
  • Add a query for cfs.

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Screenshot from 2019-06-18 22-49-22

@auto-label auto-label bot added the feature label Jun 14, 2019
@Anupam-dagar

This comment has been minimized.

Copy link
Member Author

Anupam-dagar commented Jun 14, 2019

I am not quiite sure if we can implement not available since the db field is either true or false i.e we either have it open or closed.
Also @uds5501 @shreyanshdwivedi @CosmicCoder96 can you please check it locally. For me, the events with cfs closed were also appearing in the filter of open cfs. I expect the query to be correct.
Please review.

@@ -44,6 +44,10 @@ export default Component.extend({
this.send('selectDateFilter', 'custom_dates');
},

selectEventCfs(cfs) {

This comment has been minimized.

Copy link
@mrsaicharan1

mrsaicharan1 Jun 15, 2019

Member

Just out of doubt, What is this property being used for ?

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jun 15, 2019

Author Member

to set cfs variable to either open or closed. Via this, events are filtered,

@kushthedude

This comment has been minimized.

Copy link
Contributor

kushthedude commented Jun 15, 2019

@Anupam-dagar can you please open a sub issue related to it ?

@@ -65,6 +65,18 @@ export default Route.extend({
});
}

if (params.cfs) {
let cfsVal = false;
if (params.cfs === 'open') {

This comment has been minimized.

Copy link
@CosmicCoder96

CosmicCoder96 Jun 15, 2019

Collaborator

this entire block can be reduced to a single line:

let cfsVal = (params.cfs === 'open' ? true: false);

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jun 15, 2019

Author Member

@CosmicCoder96 Requested changes completed.

@Anupam-dagar

This comment has been minimized.

Copy link
Member Author

Anupam-dagar commented Jun 15, 2019

@Anupam-dagar can you please open a sub issue related to it ?

I referenced the issue in the PR as well as commit, I don't know if it will be correct to create a sub issue for it.

@kushthedude

This comment has been minimized.

Copy link
Contributor

kushthedude commented Jun 15, 2019

@Anupam-dagar can you please open a sub issue related to it ?

I referenced the issue in the PR as well as commit, I don't know if it will be correct to create a sub issue for it.

Quoted by @CosmicCoder96
Please don't write 'partially fixes' in front of an issue number, it will still close the whole issue. If you are partially fixing a issue, then the complete issue is a parent issue, and you are fixin a sub issue, create a sub issue, and write fixes # in the PR for it.

@Anupam-dagar

This comment has been minimized.

Copy link
Member Author

Anupam-dagar commented Jun 15, 2019

@Anupam-dagar can you please open a sub issue related to it ?

I referenced the issue in the PR as well as commit, I don't know if it will be correct to create a sub issue for it.

Quoted by @CosmicCoder96
Please don't write 'partially fixes' in front of an issue number, it will still close the whole issue. If you are partially fixing a issue, then the complete issue is a parent issue, and you are fixin a sub issue, create a sub issue, and write fixes # in the PR for it.

I have added reference, writing reference doesn't close the issue, it just adds a reference there. Partially fixes closes it because it has the term fixes. You can compare it by checking a PR with Fixes in it.

If the workflow requirement is to open a subissue for it I'll open it. Waiting for @CosmicCoder96 @niranjan94 answer on it.

@Anupam-dagar Anupam-dagar force-pushed the Anupam-dagar:sidebarfilter branch from fb668eb to cbf80e3 Jun 15, 2019
@fossasia fossasia deleted a comment Jun 15, 2019
@fossasia fossasia deleted a comment Jun 15, 2019
@mariobehling

This comment has been minimized.

Copy link
Member

mariobehling commented Jun 17, 2019

@Anupam-dagar Please add screenshots showing the changes. Please always add a screenshot to PRs in the frontend. Thanks.

@fossasia fossasia deleted a comment Jun 17, 2019
@@ -35,6 +35,12 @@
<a role="button" {{action 'clearFilter' 'end_date'}}><i class="icon close"></i></a>
</div>
{{/if}}
{{#if filters.cfs}}
<div class="ui mini label">
{{if (eq filters.cfs 'true') 'Open' 'Closed'}}

This comment has been minimized.

Copy link
@CosmicCoder96

CosmicCoder96 Jun 18, 2019

Collaborator

no need to explicitly compare with true.

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jun 18, 2019

Author Member

doing {{if filters.cfs 'Open' 'Closed'}} results in only Open being set, even on selecting Closed.

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 19, 2019

Member

@Anupam-dagar then it's probably because filters.cfs is a string and not a boolean. Ensure it is a boolean.

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jun 20, 2019

Author Member

@niranjan94 I am setting it as boolean only. this.set('cfs', cfs === this.cfs ? null : (cfs === 'open' ? true : false));
Also if filters.cfs is boolean, then shouldn't the if block must not get executed when closed is selected since it will become if false.

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 21, 2019

Member

@Anupam-dagar check the data type in the model definition (if one exists). Also check if it is being assigned as a non-boolean anywhere else. Because the only explanation for the behaviour you talk about is if it is a non-boolean.

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jun 21, 2019

Author Member

@niranjan94 I refactored my code, it was being set as boolean but I interpreted it wrong in the router. A little weird thing is happening. Please take a look at the screenshot.
fossasia

Before the first click, no filter is set for cfs. I click on Open, the cfs value passed in action as parameter is true. Since cfs variable for request isn't set, this.cfs is null. The type here is boolean and Object as expected. I set the variable cfs to the param value. Now the types are boolean and boolean as expected.

Now I do the second click on the same filter to unset it, before the this.set line, the cfs param and this.cfs are both true but the types are boolean and string which should have been boolean and boolean as this.cfs was set to a boolean which can be seen from the previous console log. Now after the this.set statement, again the cfs is set to true which should have been null but since the type mismatched it is set to true. Finally the types now are both boolean.

For the third click, the type for this.cfs is boolean and hence I can unset the filter. I am not able to understand how the type is getting changed from boolean to string.

Also I am still unsure for this code block

{{#if filters.cfs}}
        <div class="ui mini label">
          {{if filters.cfs 'Open' 'Closed'}}
          <a role="button" {{action 'clearFilter' 'cfs'}}><i class="icon close"></i></a>
        </div>
{{/if}}

I tried after setting to boolean too. I am concerned about the case when filters.cfs is false, in that case will the if block get executed since the value is false. If it doesn't get executed then we won't be able to display the 'Closed' label.

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jun 24, 2019

Author Member

@shreyanshdwivedi @uds5501 @mrsaicharan1 can you please take a look here.

@Anupam-dagar Anupam-dagar force-pushed the Anupam-dagar:sidebarfilter branch from deb5fc2 to ee91f3a Jun 21, 2019
@fossasia fossasia deleted a comment Jun 21, 2019
Copy link
Collaborator

CosmicCoder96 left a comment

@Anupam-dagar When you set a query param to true, and it appears in the URL as ?xyz=true, it has been implicitly typecasted into a string. This also explains the behavior in the screenshot you added.

@Anupam-dagar Anupam-dagar force-pushed the Anupam-dagar:sidebarfilter branch from ee91f3a to c821b08 Jun 26, 2019
@Anupam-dagar

This comment has been minimized.

Copy link
Member Author

Anupam-dagar commented Jun 26, 2019

I have made the changes. I have considered true and false as string data.

@fossasia fossasia deleted a comment Jun 26, 2019
Copy link
Member

niranjan94 left a comment

I have made the changes. I have considered true and false as string data.

@Anupam-dagar this is the wrong fix. It should remain a string only in the query. Any other usage in code should be a boolean. You should do the necessary conversions for it.

@Anupam-dagar

This comment has been minimized.

Copy link
Member Author

Anupam-dagar commented Jun 27, 2019

@niranjan94 I took a look at the changes I made. I got to know that in the routes for sending a request, I need to send true or false as a string and not as a boolean. Sending it as a boolean doesn't show the correct result. Also I'm not quite sure on how I can typecast string to boolean to send to handlebar.

@niranjan94

This comment has been minimized.

Copy link
Member

niranjan94 commented Jun 28, 2019

@Anupam-dagar see https://guides.emberjs.com/v3.10.0/routing/query-params/#toc_default-values-and-deserialization

Query param values are cast to the same datatype as the default value. The same also applies to boolean default values.

@fossasia fossasia deleted a comment Jun 28, 2019
@Anupam-dagar

This comment has been minimized.

Copy link
Member Author

Anupam-dagar commented Jun 28, 2019

@niranjan94 I tried by setting a default value, it didn't seem to work as in that case I had to make a decision to what type of events I should show by default.
I got rid of the boolean datatype. All the operations now happen within string datatype. The true and false boolean for query param is decided on the basis of cfs type Open or Closed.

@fossasia fossasia deleted a comment Jun 28, 2019
@@ -76,6 +76,14 @@ export default Route.extend({
val : `%${params.location}%`
});
}
if (params.cfs) {
let cfsVal = params.cfs === 'Open' ? true : false;

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 29, 2019

Member

Use lower case open/close

filterOptions.push({
name : 'is_sessions_speakers_enabled',
op : 'eq',
val : cfsVal

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 29, 2019

Member

This can just be params.cfs === 'open'

This comment has been minimized.

Copy link
@kushthedude

kushthedude Jun 30, 2019

Contributor

@Anupam-dagar You missed this one!

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jul 1, 2019

Author Member

@kushthedude this has also been fixed, check in changes section.

@fossasia fossasia deleted a comment Jun 29, 2019
@Anupam-dagar

This comment has been minimized.

Copy link
Member Author

Anupam-dagar commented Jun 29, 2019

@niranjan94 This is ready for another review. I guess it's complete now.

@niranjan94 niranjan94 requested a review from CosmicCoder96 Jul 1, 2019
@fossasia fossasia deleted a comment Jul 4, 2019
hideClearFilters: computed('category', 'sub_category', 'event_type', 'startDate', 'endDate', 'location', 'ticket_type', function() {
return !(this.category || this.sub_category || this.event_type || this.startDate || this.endDate || this.location || this.ticket_type !== null);
hideClearFilters: computed('category', 'sub_category', 'event_type', 'startDate', 'endDate', 'location', 'ticket_type', 'cfs', function() {
return !(this.category || this.sub_category || this.event_type || this.startDate || this.endDate || this.location || this.ticket_type || this.cfs !== null);

This comment has been minimized.

Copy link
@CosmicCoder96

CosmicCoder96 Jul 4, 2019

Collaborator

if you are passing a string to query param cfs, you can just use || this.cfs here?

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jul 5, 2019

Author Member

the other data type are also string, hence i added it in this way.

This comment has been minimized.

Copy link
@CosmicCoder96

CosmicCoder96 Jul 5, 2019

Collaborator

this.cfs !== null is true if cfs is not null, false if it is null.
so you can just write this.cfs, there is no need for the explicit comparison to null

filterOptions.push({
name : 'is_sessions_speakers_enabled',
op : 'eq',
val : cfsVal

This comment has been minimized.

Copy link
@CosmicCoder96

CosmicCoder96 Jul 4, 2019

Collaborator
val: params.cfs === 'open'
@Anupam-dagar Anupam-dagar force-pushed the Anupam-dagar:sidebarfilter branch from 4a8f7e2 to 8470d3e Jul 5, 2019
@fossasia fossasia deleted a comment Jul 5, 2019
hideClearFilters: computed('category', 'sub_category', 'event_type', 'startDate', 'endDate', 'location', 'ticket_type', function() {
return !(this.category || this.sub_category || this.event_type || this.startDate || this.endDate || this.location || this.ticket_type !== null);
hideClearFilters: computed('category', 'sub_category', 'event_type', 'startDate', 'endDate', 'location', 'ticket_type', 'cfs', function() {
return !(this.category || this.sub_category || this.event_type || this.startDate || this.endDate || this.location || this.ticket_type || this.cfs !== null);

This comment has been minimized.

Copy link
@CosmicCoder96

CosmicCoder96 Jul 5, 2019

Collaborator

this.cfs !== null is true if cfs is not null, false if it is null.
so you can just write this.cfs, there is no need for the explicit comparison to null

@Anupam-dagar

This comment has been minimized.

Copy link
Member Author

Anupam-dagar commented Jul 6, 2019

@CosmicCoder96 @niranjan94 Updated pull request with the requested changes. Please review.

@fossasia fossasia deleted a comment Jul 6, 2019
@niranjan94 niranjan94 merged commit 1415ddb into fossasia:development Jul 6, 2019
7 checks passed
7 checks passed
Header rules No header rules processed
Details
Pages changed 6 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.