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

Fix updated api not returning any item after marking item as read #1713

Merged
merged 1 commit into from
May 2, 2022

Conversation

Grotax
Copy link
Member

@Grotax Grotax commented Mar 30, 2022

The updated items api is supposed to return updated items, the last modified date is used for that.

I tried to use the $time provided through dependency injection but for some reason it is null.

I don't really care if we can't use dep injection there, if I made a mistake though I would be happy to adjust it.

@Grotax Grotax force-pushed the fix/lastmodified branch from 1417725 to a053f2a Compare March 30, 2022 17:36
@Grotax
Copy link
Member Author

Grotax commented Mar 30, 2022

seems like bats is broken, sometimes ${BATS_SUITE_TEST_NUMBER} is empty

@Grotax
Copy link
Member Author

Grotax commented Mar 31, 2022

I don't know how to fix the test, I guess you need to mock the time object to return a fixed value but I don't know how.

@anoymouserver
Copy link
Contributor

anoymouserver commented Mar 31, 2022

Maybe like this? (only getMicroTime() instead)

$this->time = 222;
$this->autoPurgeMinimumInterval = 10;
$timeFactory = $this->getMockBuilder(Time::class)
->disableOriginalConstructor()
->getMock();
$timeFactory->expects($this->any())
->method('getTime')
->will($this->returnValue($this->time));

(I didn't look at the test though, just saw your question.)

@Grotax
Copy link
Member Author

Grotax commented Mar 31, 2022

Well I managed to make the test working, it just checks if the parameter is given but not the value.
Which is maybe also not so important. I tried to make the time a fixed value by creating a Mock similar to the db object.
And then tired to replace the return value of the method that is used but in the test result the value did not match.

@Grotax
Copy link
Member Author

Grotax commented Mar 31, 2022

@anoymouserver thanks :)

I guess like it is done now also works. No need to test the function with a specific time.
And my new api tests actually test if this works the way it is expected at api level. :)

@Grotax
Copy link
Member Author

Grotax commented Apr 14, 2022

I'm basically done with this, it fixes this specific error.
To fully fix the API the date would need to be updated whenever an item is changed in some way.

Any opinions?
Also related to #1727

@Grotax Grotax force-pushed the fix/lastmodified branch from 45800a2 to 814a591 Compare April 30, 2022 10:24
@Grotax
Copy link
Member Author

Grotax commented Apr 30, 2022

rebased on master

@Grotax
Copy link
Member Author

Grotax commented May 2, 2022

If there is no negative feedback I would just squash the commits and merge it.

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>

this way it works

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>

add changelog entry

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>

Partly fix test

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>

test passing

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax force-pushed the fix/lastmodified branch from 814a591 to 31da758 Compare May 2, 2022 07:55
@Grotax Grotax merged commit 5af0cb6 into master May 2, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/lastmodified branch May 2, 2022 07:56
Grotax added a commit that referenced this pull request May 29, 2022
Changed
- Add API v1.3 adding routes for starring/unstarring items by id and general fixes (#1727)
  https://nextcloud.github.io/news/api/api-v1-3/
- Improve styling of tables in articles (#1779)
- Allow fetching feeds that omit guid by using link as stand-in (#1785)

Fixed
- Fix updated api not returning any item after marking item as read (#1713)
- Fix deprecation warning for strip_tags() on a null value (#1766)
- Fix selected item being set incorrectly when using default ordering or newest first ordering (#1324)
- Fix doubling the height of the content area (#1796)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax Grotax mentioned this pull request May 29, 2022
Grotax added a commit that referenced this pull request May 29, 2022
Changed
- Add API v1.3 adding routes for starring/unstarring items by id and general fixes (#1727)
  https://nextcloud.github.io/news/api/api-v1-3/
- Improve styling of tables in articles (#1779)
- Allow fetching feeds that omit guid by using link as stand-in (#1785)

Fixed
- Fix updated api not returning any item after marking item as read (#1713)
- Fix deprecation warning for strip_tags() on a null value (#1766)
- Fix selected item being set incorrectly when using default ordering or newest first ordering (#1324)
- Fix doubling the height of the content area (#1796)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants