search

marius-wieschollek / passwords

A simple, yet feature rich password manager for Nextcloud
GNU Affero General Public License v3.0
215 stars 45 forks source link

Made check pwd job time insensitive #580

Closed akhil1508 closed 1 year ago

akhil1508 commented 1 year ago

Resolves https://github.com/marius-wieschollek/passwords/issues/579

marius-wieschollek commented 1 year ago

Have you tested this? Because the OC\BackgroundJob\TimedJob doesn't provide this function. We need to switch to OCP\BackgroundJob\TimedJob for this.

Also please sign your commits as required, see Verify Git Commits.

akhil1508 commented 1 year ago

Have you tested this? Because the OC\BackgroundJob\TimedJob doesn't provide this function. We need to switch to OCP\BackgroundJob\TimedJob for this.

Makes sense. My bad, I thought a small enough change and didn't really test it out. Didn't notice that. Let me check the nextcloud source code to see if these two classes are very different functionality wise.

Also please sign your commits as required, see Verify Git Commits.

Done

akhil1508 commented 1 year ago

@marius-wieschollek It looks fine to just make the switch to OCP\BackgroundJob\TimedJob;. The other one is a deprecated class for "internal use" as per a comment.

marius-wieschollek commented 1 year ago

Now i get an error show for the $interval variable. This is defined hardcoded in line 41 and overwritten in SendServerSurvey, CheckNightlyUpdates and CheckPasswordsJob. Also there seems to be a dependency to ITimeFactory $time in the constructor of the Job class which isn't fulfilled because the constructor isn't called and the AbstractTimedJob (and all that extend it) don't require it.

akhil1508 commented 1 year ago

Now i get an error show for the $interval variable. This is defined hardcoded in line 41 and overwritten in SendServerSurvey, CheckNightlyUpdates and CheckPasswordsJob. Also there seems to be a dependency to ITimeFactory $time in the constructor of the Job class which isn't fulfilled because the constructor isn't called and the AbstractTimedJob (and all that extend it) don't require it.

@marius-wieschollek Got it, I'll check on how to make it compatible correctly and actually run it and test.. My bad on assuming it "should" work out of the box, sorry :(