Skip to content

Conversation

@3on108
Copy link
Contributor

@3on108 3on108 commented Aug 29, 2021

Summary

Added cron job to permanently delete deck cards 5 min after they are marked as deleted

TODO

  • ...

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your pull request. I've made some comments on the code, but the general approach is fine 👍

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks code code wise, just some small nitpicking on the code style :)

@matejak
Copy link

matejak commented Oct 25, 2021

@juliushaertl Hello, could you please check this out again? This seems to be a really useful PR.
I would like to help with the review, but I need to point out to some documentation regarding how to get a development version of the app into a testing server.

@juliusknorr
Copy link
Member

It seems the tests need some additional adjustments:

 There were 2 errors:

1) OCA\Deck\Cron\DeleteCronTest::testDeleteCron
PHPUnit\Framework\MockObject\UnknownTypeException: Class or interface "OCA\Deck\Cron\CardMapper" does not exist

/home/runner/work/deck/deck/apps/deck/tests/unit/Cron/DeleteCronTest.php:50

2) OCA\Deck\Cron\DeleteCronTest::testDeleteCronInvalidAttachment
PHPUnit\Framework\MockObject\UnknownTypeException: Class or interface "OCA\Deck\Cron\CardMapper" does not exist

/home/runner/work/deck/deck/apps/deck/tests/unit/Cron/DeleteCronTest.php:50

--

@matejak
Copy link

matejak commented Nov 8, 2021

@juliushaertl It would be really great to provide some kind of documentation for developers so they can set up the environment locally and test or fine-tune the development work using the fast-feedback loop of local testing instead of iterating based on the PR gating results.
Is the documentation really missing, or does it exist somewhere outside of the doc folder?

@juliusknorr
Copy link
Member

You're right, there is no specific developer documentation for deck, maybe you can open an issue for that so we can add something in the future. In general the local Nextcloud developer documentation should apply for most of the parts:

https://docs.nextcloud.com/server/latest/developer_manual/getting_started/devenv.html

Elsewise you also might be interested in https://github.com/juliushaertl/nextcloud-docker-dev if you perfer a out-of-the box docker based environment.

@3on108
Copy link
Contributor Author

3on108 commented Nov 10, 2021

Thank you @juliushaertl for triggering the tests and information about the documentation. I will try to set-up deck to run in my local docker. Meanwhile I've fixed another issue causing the tests to fail. Can you please retrigger them? Thanks.

@3on108
Copy link
Contributor Author

3on108 commented Jan 24, 2022

Hi @juliushaertl,

I've found and fixed a bug in my code changes to the CardMapper. Can you please rerun the tests?

I've managed to setup nextcloud running from a docker image, however did not manage to install deck there. Simple copying of its files did not help. Neither did I manage to run unit tests there. Is there some handbook to follow how to do that properly?

Thanks and kind regards.

@jivanpal
Copy link

Can we re-run the tests on this to get current failure logs?

@marcelklehr
Copy link
Member

marcelklehr commented Oct 27, 2022

You should be able to trigger a new test run by rebasing and force pushing. Unfortunately I'm unable to rerun the tests atm

Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@jivanpal
Copy link

@marcelklehr Thanks for merging so that the tests would run. I am interested in fixing this so that it can be merged, but this is not my PR, so I cannot write to the PR branch. Would you recommend that I fork it and create another PR to do this?

The PHP error should be trivial to resolve, but I have no idea how to fix the Cypress tests, and we probably still need the author @3on108 to sign-off their commits, right?

@3on108
Copy link
Contributor Author

3on108 commented Oct 28, 2022

Hi @jivanpal, great you took this issue over. I got stuck on setting up nextcloud and deck to run locally or at least the tests. I can assist with anything needed. Not sure what signing-off commits, I'm a bit newbie to this.

@juliusknorr
Copy link
Member

Pushed a rebased version of this with a fix for the test to #4194 since I couldn't push to the original branch.

@juliusknorr juliusknorr closed this Nov 7, 2022
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.

Deleted deck cards are being kept in deleted items forever

5 participants