search

genouest / genouestaccountmanager

Account manager for core facility
GNU Affero General Public License v3.0
5 stars 8 forks source link

Extended tp registrations #465

Closed rsiminel closed 3 months ago

rsiminel commented 3 months ago

I still have a couple questions that I included as comments in the code. While I'm at it, here are a couple more concerns : While looking arround, I noticed that when a user wants to extend a Project, it is automatically extended by one year. Was I just supposed to do something like that ? Or should I make the Project extention more like this one ? Also, should I add checks that TPs/Projects are not created retroactively (start/end date < current date) ?

closes #453

mboudet commented 3 months ago

I noticed that when a user wants to extend a Project, it is automatically extended by one year. Was I just supposed to do something like that

Wow, I did not know this option existed (it's disabled on our instance). No it's fine to leave it that way, extension by an user and by an admin are two separate thing.

Should I add checks that TPs/Projects are not created retroactively (start/end date < current date) ?

If it's not already implemented, sure.

Now that I'm looking at the code, I think you'll need to extend the 'expiration' value of created TP account (if any) to match the new expiration date + the grace period

rsiminel commented 3 months ago

Ok weird, I somehow edited your comment instead of adding one... sorry about that

mboudet commented 3 months ago

PS: Some of the files I'm working on are getting pretty long (500 lines +), would it be useful to try using JavaScript Regions ?

This is a vscode specific comment I believe? No really a fan of editor-specific comments in the code.

Ps: 96e96d8 broke the tests I think?

rsiminel commented 3 months ago

Sorry about the broken tests, pesky semicolons ;)

PS: When a user does not fill in the "About" section when reserving a TP, the error message 'Tell us why you need some tp accounts' does not appear on the webpage until something is actually written in the "About" section. Shall I make a new issue for this or is it an easy fix? Also, that error and the 'Quantity must be >= 1' error are only tested for in the backend.

EDIT: The error I mention in my PS is actually Issue #447

rsiminel commented 3 months ago

Yeah, the test that we had for TP booking and creation did everything in a single test case that was written in a peculiar way. I made it look like all the others and added test cases for TP extending and removing. Some of the other tests should perhaps be a little more restrictive than just making sure you get an HTTP status of 200 but that's an issue for an other day.