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

Device expiry implementation #57

Merged
merged 1 commit into from
Apr 1, 2024
Merged

Device expiry implementation #57

merged 1 commit into from
Apr 1, 2024

Conversation

skizzofly
Copy link
Contributor

@skizzofly skizzofly commented Mar 7, 2024

This modification, permits a device expiration if after one year the record into the OGN DB is not updated.
To renew another year the owner has to go under update, click the check box that he owns the device and save, this will lock the device for the next 365 days.
The expiration status is shown on the device list table, green check is still valid, red X is expired and can be transferred.
Once expired, the device is free to be transferred to an other owner, this to automate device transferring due to inactive accounts
The API's are untouched, a expired device will still show up.

This modification should also loose some work of the admin team, if a flarm owner is really dead or sold the glider, the new owner has to wait for expiry, he will not know if it expired or not, but he can time to time try to transfer the device, if it's denied, means device is still in force of the old user.
ogn_mod
database_schema_upgrade.sql.txt
INSTALL.md

@snip
Copy link
Contributor

snip commented Mar 7, 2024

image A cross means it is expired or not? This seems not very straightforward in logic.

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 7, 2024

Well, a bad red cross means expired (red cross BAD / green check OK), seems quite logic. to me :)
The column is named Not Expired, if it's Green it's not expired, if it's red , it's expired.
Can evaluate if call the column Live or if you prefer, but Unexpired is not a nice label, Not expired is quite explicit

@snip
Copy link
Contributor

snip commented Mar 7, 2024

Well, a bad red cross means expired (red cross BAD / green check OK), seems quite logic. to me :) The column is named Not Expired, if it's Green it's not expired, if it's red , it's expired. Can evaluate if call the column Live or if you prefer, but Unexpired is not a nice label, Not expired is quite explicit

What i see, it is is not clear at all. Users are going to be completly lost with this.
More over it is not an option of the configuration like the other chek/cross but a status. So it should be more at the begining of the line i think.
Maybe we need to find other icons ...

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 7, 2024 via email

@snip
Copy link
Contributor

snip commented Mar 7, 2024

So tell me how you would implement this view..

On Thu, Mar 7, 2024 at 7:07 PM Sebastien Chaumontet < @.> wrote: Well, a bad red cross means expired (red cross BAD / green check OK), seems quite logic. to me :) The column is named Not Expired, if it's Green it's not expired, if it's red , it's expired. Can evaluate if call the column Live or if you prefer, but Unexpired is not a nice label, Not expired is quite explicit What i see, it is is not clear at all. Users are going to be completly lost with this. More over it is not an option of the configuration like the other chek/cross but a status. So it should be more at the begining of the line i think. Maybe we need to find other icons ... — Reply to this email directly, view it on GitHub <#57 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL3TFMKRTTJQQ4YV5OZLV2LYXCUH3AVCNFSM6AAAAABELKUZFCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBUGE2DCNJVGY . You are receiving this because you authored the thread.Message ID: @.>

Some ideas of icons: https://www.google.com/search?q=expired&tbm=isch&chips=q:expired,g_1:icon:t4tNJzcaTj4%3D&rlz=1C5CHFA_enFR914FR914&hl=en&sa=X&ved=2ahUKEwjt49aX3uKEAxXwuCcCHSDNBlwQ4lYoBnoECAEQPQ&biw=1680&bih=963
But we need something free to use.

We need to agrre on something usable before implementing it. So feel free to propose options.

@skizzofly
Copy link
Contributor Author

This states free and seems ok
License: All icons on this site can be used in personal, commercial, and client projects without any attribution or credit. License page

Valid not expired
https://uxwing.com/on-time-icon/

Expired icon
https://uxwing.com/delay-icon/

intime
expired

They are SVG, so they can be big or small.. i resized them to 20x16

@snip
Copy link
Contributor

snip commented Mar 7, 2024

This states free and seems ok License: All icons on this site can be used in personal, commercial, and client projects without any attribution or credit. License page

Valid not expired https://uxwing.com/on-time-icon/

Expired icon https://uxwing.com/delay-icon/

intime expired

They are SVG, so they can be big or small.. i resized them to 20x16

Looks nice.
So what should be the column title? "Expired" ?
I think we should also implement a tooltip info when mouse is over to explain what it is.

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 7, 2024 via email

@snip
Copy link
Contributor

snip commented Mar 9, 2024

Hi @skizzofly do you have time to complete updates of this PR?

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 9, 2024 via email

@snip
Copy link
Contributor

snip commented Mar 9, 2024

"Expiration" and the 2 icons looks ok to me yes.

@skizzofly
Copy link
Contributor Author

Ok I also added the tooltip for the 2 cases
Valid is ok for a "non expired" device,
Or you preferer a more simple tooltip, for both cases

ogn_exp_screen

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 9, 2024 via email

@snip
Copy link
Contributor

snip commented Mar 9, 2024

Can you please move this column between the edit and trash button as it is not parameters?

Please also fix other comments.

@snip
Copy link
Contributor

snip commented Mar 9, 2024

Instead of "The current device is: Valid" we can say: "This device will expire in x days". What do you think?

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 9, 2024 via email

@skizzofly
Copy link
Contributor Author

Here is the new layout with the moved column
ogn_mod1
Ok it's ok i will commit it.

@snip
Copy link
Contributor

snip commented Mar 9, 2024

It looks nice.

@snip
Copy link
Contributor

snip commented Mar 9, 2024

About the expiration date i think it needs to be known by the user. So without calculating the expiration date can be write in the tooltip.

@skizzofly
Copy link
Contributor Author

This is ok?
ogn_exp_date

@snip
Copy link
Contributor

snip commented Mar 9, 2024

Maybe a sentence?
"This device registration is going to expire the 7th of March 2025"
(with month in letters to prevent US/EU month/day inversion confusion in date).

Edit: added registration keyword

@snip
Copy link
Contributor

snip commented Mar 9, 2024

A very nice thing would be to be able to click on the device to renew the expiration for one year.
(so in the tooltip we can add a message "click here to renew it for one year").
But maybe not so easy to implement.

At least we need some explicit message somewhere to explain how to renew the device. And also explain what is the risk of device expired.

@skizzofly
Copy link
Contributor Author

I think now it's ok

ogn_date_expiry

Regarding the update, it's correct that the user goes in Edit, reviews the info, and more important flags the
"I certify to be the owner of this device" to save back the info.

Yes we can write a disclamier someware.

Apart these minor cosmetic issues, did you give an eye to the logic?

@snip
Copy link
Contributor

snip commented Mar 10, 2024

Apart these minor cosmetic issues, did you give an eye to the logic?

Qucikly but you need to comit you change, fix other reported small issues, then squash and finally push.
Then we can get a better view.

@skizzofly
Copy link
Contributor Author

Good Sunday Seb,
Check if this disclaimer is ok.. if not, please suggest a adeguate text.
I also added the registration in the tooltip.
If all is ok I will commit and figure out how to squash without making a mess.

ogn_date_expiry_disclaimer

@snip
Copy link
Contributor

snip commented Mar 10, 2024

You are going to the good way i think :)

Maybe adding a general information message at the bottom.
And if there is expired device in the list maybe add at top a warning message in red saying that and with quick explanation.

@skizzofly
Copy link
Contributor Author

Good evening,
I think now it's ok
If a User ha no expired devices, it will compare as usual.. (first pic)
If a User has and expired device, when a red WARNING will apper, and on the bottom a text message explaining how to renew the device. (second pic)
Check if the text is correct, if not please write me what is correct to you to appear.
Ale.

ogn_expired_or_not

@snip
Copy link
Contributor

snip commented Mar 10, 2024

When you speak about device expired, can you please replace it by device registration expired?

Can you please keep the sentence at the bottom of the table in all cases? Can you please remove the Warning for it?

@skizzofly
Copy link
Contributor Author

Ok, now i really hope it's all ok
ogn_date_expiry_new

@skizzofly
Copy link
Contributor Author

Seb, i am sorry but i really do not see the repeorted issues..
do not see them in the comments of the changed files and not in the Review section
Regarding the indentation and time constant issues i commited and squased in to them in to 979f9a0 3 days ago.
Pleas tell me where You reported the issues..

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 15, 2024 via email

@snip
Copy link
Contributor

snip commented Mar 15, 2024

You did not commit anything since my last message / last check.
See the begingin of this conversation for review part. #57

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 15, 2024 via email

index.php Outdated Show resolved Hide resolved
index.php Outdated Show resolved Hide resolved
language/english.php Outdated Show resolved Hide resolved
index.php Outdated Show resolved Hide resolved
index.php Outdated Show resolved Hide resolved
language/dutch.php Outdated Show resolved Hide resolved
language/english.php Outdated Show resolved Hide resolved
language/french.php Outdated Show resolved Hide resolved
language/german.php Outdated Show resolved Hide resolved
templates/devicelist.html.twig Outdated Show resolved Hide resolved
@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 15, 2024 via email

@snip
Copy link
Contributor

snip commented Mar 15, 2024

Sorry for that! I thought you were seeing it as you reply on some of them. But it was maybe just my screenshot.

@skizzofly
Copy link
Contributor Author

skizzofly commented Mar 15, 2024 via email

@skizzofly
Copy link
Contributor Author

Hi with e959115 i commited the changes requested, if it's all ok i will squash it

Please note that in devicelist.html.twig the TD's in the first part of the tables are all lowercase, and in the second part of the table they are uppercase, i am not sure if there is a logic in this, anyway i did as you told me, lowering the case of my addon.

Copy link
Contributor Author

@skizzofly skizzofly left a comment

Choose a reason for hiding this comment

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

commited with e959115

index.php Outdated Show resolved Hide resolved
@skizzofly
Copy link
Contributor Author

Seb, did you check my commits?
If they are OK I will squash the commits as usual..

@snip
Copy link
Contributor

snip commented Mar 21, 2024

Seb, did you check my commits? If they are OK I will squash the commits as usual..

There is still 2 small reviews to fix.
Can you please check?

@skizzofly
Copy link
Contributor Author

Seb, if the fixes are regarding the comment and the 15 month i commited it with aec3279

ogn_comment_fix

IF the changes are other, i do not see them, so be sure to submit them..

@snip
Copy link
Contributor

snip commented Mar 22, 2024

I think just this one is still missing:
image

@skizzofly
Copy link
Contributor Author

fixed with 8ebe58f
Check, if all is OK i will squash it up..
Ceers.

@snip
Copy link
Contributor

snip commented Mar 23, 2024

It seems we are now good!
Go for squash!

Device expiry implmentation

Cosmetic fixes

Device expiry, cosmetic fixes

centralized expireseconds, fix indent code

made costant expirationdelta

Code Review commit

Added comments, and modified from 12 to 15 months the expirationdelta

brace and comment fx
@skizzofly
Copy link
Contributor Author

squashed into 6d2f41b

@snip
Copy link
Contributor

snip commented Mar 26, 2024

@skizzofly about the database_schema_upgrade.sql.txt can you please provide a version which randomize by 2 months the expiration date, so it will not be predictable (someone will not be able to try all device to get it for his own at the initial expiry time)?
Thanks
Everything else looks ok so far.

@skizzofly
Copy link
Contributor Author

Seb, the dev_updatetime is NOT the expiry date , but the timestamp that the device was updated.
But Seb, really you are telling me that there are people everyday trying to steal legittime device ID's so it's so VITAL to randomize the expiry date by two month?
IMHO it's a useless waste of development time, as if the device is not free today i will try tomorrow, and if not after tomorrow, and if not in a week.. until it will be free.
Anyway I really have no idea how to randomize a number in mysql in a certain range..sorry for that.. :(
Ale.

@skizzofly
Copy link
Contributor Author

As required.. generates some random numbers around the current time stamp.
If you have a better solution be free to implement it.. as mentioned before i am not so good in SQL.

database_schema_upgrade.sql.txt

@snip
Copy link
Contributor

snip commented Mar 26, 2024

But Seb, really you are telling me that there are people everyday trying to steal legittime device ID's so it's so VITAL to randomize the expiry date by two month?

Not everyday, bit once this will be announced, some will see this and maybe put an alarm for the D day next year to be able to get lot of ownership.

@snip
Copy link
Contributor

snip commented Mar 26, 2024

As required.. generates some random numbers around the current time stamp. If you have a better solution be free to implement it.. as mentioned before i am not so good in SQL.

database_schema_upgrade.sql.txt

Thanks!

I'm going to try this.

@flightenthusiast
Copy link

@skizzofly thank you for your contribution and what you are doing

@skizzofly
Copy link
Contributor Author

@flightenthusiast , maybe this will solve a long timed problem.. our club has such a problem with a glider registered in south africa.. the owner is disappeared and does not reply to mails and no one knows where he is , we tried to contact him but there is no trace of him.. they sold us his device leggily (via LXNav and we have the invoice) but still is registered.
With this mod problems like this may be solved automatically.

@snip try the script on a copy of the DB before going in production . ;)

Ceers.

@snip snip merged commit 612703b into glidernet:master Apr 1, 2024
@snip
Copy link
Contributor

snip commented Apr 1, 2024

I just put it in production. Thanks a lot @skizzofly.

@skizzofly
Copy link
Contributor Author

skizzofly commented Apr 1, 2024 via email

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.

3 participants