Added Option to export csv between dates#101
Conversation
WillStrohl
left a comment
WillStrohl
left a comment
There was a problem hiding this comment.
Overall, this is a fantastic PR. Thank you for taking the time to make the contribution. This is especially true since this module is still using a legacy development pattern. Good stuff! :)
I left a few notes about some minor things, but these minor things could reduce errors later when this is in the next release.
Again, well done.. The DNN community thanks you! :)
| </div> | ||
| <div class="dnnFormItem"> | ||
| <dnn:label id="plInitialDate" runat="server" controlname="cbSystemFields" /> | ||
| <asp:TextBox ID="txtInitialDate" CssClass="NormalTextBox" runat="server" MaxLength="200" |
There was a problem hiding this comment.
There's no validation to prevent errors and user experience issues. Also, the readonly attribute should probably be here and not only in the JavaScript, so that it's still readonly if JS is turned off - unless you have a technical reason for that. :)
There was a problem hiding this comment.
@ufoloko So you're not able to validate that the value is in a date format or empty? Maybe with a cominbation of Required, Custom, and Range validators to ensure the dates are in sequence and if one is entered, the other needs to be entered or deleted?
| var ds = new UserDefinedTableController(_moduleId, TabId, UserInfo).GetDataSet(true); | ||
| DataSet ds; | ||
|
|
||
| if(initialDate.Length>0 && finalDate.Length > 0) |
There was a problem hiding this comment.
This probably needs to be changed, but just as a note for performance for code in general, this might be better done using the built-in string.IsNullOrEmpty() method, because these are string values, and length could return a value greater than zero if there is one or more space values.
There was a problem hiding this comment.
Yes you are right. Sorry im not so experienced about how github works, how i can make this change to improve this PR.
There was a problem hiding this comment.
No worries... In the same branch you used to create the PR, just make the update and then push again as a new commit. This will update the PR for you. :) Nothing more is needed.
|
@ufoloko @WillStrohl if you want to get our head together to get this merged and do a new release, we are doing open-source co-coding events each Saturday on Discord, you are welcome to join us to get this PR pushed through the finish line and getting a new release out. https://discord.gg/m4RaJWd8 |
|
Finally I have added suggestions you sent. About readonly feature, I need to add that option in client side because if you add that option from server, any change you make in dates is not being saved. I have added the option from server side but adding attributes to fields. |
valadas
left a comment
valadas
left a comment
There was a problem hiding this comment.
This looks good to me, I'll merge and we can iron out anything that crops up in other PRs.
3 participants
I have added an option to export to csv for data between two dates.
This works adding a timestamp field for UserDefinedRows table to be able to get records between two dates. This field is automatically populated when new row is added.
If TWO date fields are set in export section, results are filtered, otherwise all records will be exported.
Closes #100