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

Date filters for short urls list endpoint #572

Merged
merged 18 commits into from
Dec 17, 2019

Conversation

alesub
Copy link
Contributor

@alesub alesub commented Dec 11, 2019

These are new filters for the GET /rest/v1/short-urls endpoint. startDate and endDate are added as parameters to limit the results. It is useful to build reports on applications where the service has been running for several months.

Closes #575

@acelaya
Copy link
Member

acelaya commented Dec 11, 2019

Cool! This will be really useful 😁

However, some changes are needed:

  • This branch seems to be outdated. Conflicts need to be solved before merge is even possible.
  • In most of the places, shlink makes use of Chronos objects instead of DateTimes. They are immutable and more flexible, so I would type hint to those any new date argument.
  • The API always expects ISO dates in any other endpoint where a date can be provided, so I would also expect that format here, for consistency. It would also let more granular filtering than the Y-m-d format.
  • Coding styles need to be fixed (usually GitHub triggers a build when a PR is created
    In this case it didn't happen. Maybe because of the conflicts? 🤔). In order to do it, it's usually enough to run composer cs:fix.
  • Some tests need to be added (this is probably the most complex part). In this case we need database tests (they are basically integration tests which run against a real database covering repositories) and API tests (these are end-to-end tests running against real API endpoints).
  • Open API docs need to be updated, including these two new arguments.

Of course, you can choose not to do any of those points, just do some of them, or do everything. I leave it up to you. I will finish what's missing 😀

And thanks again for the contribution!

@alesub
Copy link
Contributor Author

alesub commented Dec 11, 2019

Thanks for the feedback @acelaya, I'll see how much of this I can do and will let you know about any updates.

@alesub
Copy link
Contributor Author

alesub commented Dec 15, 2019

Hi @acelaya, I have applied all requests but the tests, if that's ok I'll leave the tests to you.

Please let me know about any comments,
Thanks!

@acelaya
Copy link
Member

acelaya commented Dec 16, 2019

Amazing! Thanks for the hard work :)

If I remember correctly, github should allow me to push commits to your fork's branch, but if not, I will fork your fork :)

I will try to do it in the next days, in order to include this in v1.21

@acelaya acelaya self-requested a review December 16, 2019 14:17
@acelaya acelaya dismissed their stale review December 16, 2019 14:17

Changed applied

@alesub
Copy link
Contributor Author

alesub commented Dec 16, 2019

Sounds great, thanks to you for this great URL shortener @acelaya!

@acelaya
Copy link
Member

acelaya commented Dec 16, 2019

Confirmed. Github lets me push to your fork. And build is back at green :)

@acelaya
Copy link
Member

acelaya commented Dec 16, 2019

Just added all unit/database/api tests and fixed a couple of issues found in the process.

Tomorrow I will add support for these two new arguments to be passed to the short-url:list command, and then the PR will be ready to merge.

@alesub
Copy link
Contributor Author

alesub commented Dec 16, 2019

That's amazing, thanks!

@acelaya
Copy link
Member

acelaya commented Dec 17, 2019

@alesub everything is ready now. You can review it if you want, but I'll probably merge it as soon as builds pass.

@acelaya
Copy link
Member

acelaya commented Dec 17, 2019

I will also start working on shlink-web-client v2.3 after finishing shlink's v1.21 milestone, which will include controls to filter short URLs list by date, making use of this new implementation 😃

shlinkio/shlink-web-client#178

@acelaya acelaya merged commit 685b3f8 into shlinkio:develop Dec 17, 2019
@alesub
Copy link
Contributor Author

alesub commented Dec 17, 2019

Amazing! Thanks!!

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.

Allow short URL lists to be filtered by date range
2 participants