fortran-lang / registry

Registry for Fortran package manager
MIT License
8 stars 3 forks source link

feat: extract homepage and repository and description from fpm.toml #36

Closed henilp105 closed 1 year ago

henilp105 commented 1 year ago

Link for Testing https://registry-phi.vercel.app/

Thanks and Regards, Henil

CC: @awvwgk @arteevraina @minhqdao @perazz

arteevraina commented 1 year ago

@henilp105 Can you provide with testable link ?

henilp105 commented 1 year ago

Sure @arteevraina , here is a testable link : https://registry-phi.vercel.app/

arteevraina commented 1 year ago

Hey @henilp105 , Thanks for the PR.

I have reviewed your Pull Request. I think other than the comments above. I think admin panel needs more work. There are some issues that I have noticed but I could screenshot them because they animated for a few seconds like buttons getting expanded abnormally and screen after clicking buttons unnecessarily. I also noticed admin panel forms should also have validation on the frontend so that every API request that reaches the server has the correct data atleast.

henilp105 commented 1 year ago

@arteevraina , I had noticed that button and animation bug , it is a bug in mdb-react library ( refer: https://github.com/mdbootstrap/mdb-ui-kit/issues/1521 , https://github.com/mdbootstrap/mdb-ui-kit/issues/1505 ) , I couldn't find a suitable patch for this bugs. Sure , will add validation for the admin panel.

minhqdao commented 1 year ago

After logging in, I now see first see a loading indicator on the left, then my namespace and packages showing, then I see another loading indicator in the middle, then everything seems to refresh again.

minhqdao commented 1 year ago

Can I change my email now? I can't see how.

minhqdao commented 1 year ago

How can I test the email verification? I signed up using a new email address, but there was no verification process triggered.

henilp105 commented 1 year ago

@arteevraina , @minhqdao , @perazz Can you please review the PR ?

Testable link: https://registry-phi.vercel.app/

perazz commented 1 year ago

Thank you @henilp105. I did register a new test account and I could receive the verification email. Tested email change and it did work. What is supposed to happen when verification is not completed yet? Should the user still be able to use their accounts and upload packages? Same when the user changes email address.

henilp105 commented 1 year ago

Thanks @perazz , I have patched that now the user will not be allowed to access the dashboard or any functionality like change email, forgot password, upload a package / create namespace until the email is not verified. ( non verified user won't be able to perform any action , non verified user can only be deleted by sudo admins ) .

henilp105 commented 1 year ago

I have also added the functionality for the archives from the registry , automatically generated at weekly intervals which can be retrieved from the frontend.

minhqdao commented 1 year ago

I have also added the functionality for the archives from the registry , automatically generated at weekly intervals which can be retrieved from the frontend.

1) Could you please explain in more detail what this means? Are we adding new packages to the archive every week and changing the edited ones? Or are we generating weekly snapshots of the entire database?

2) Maybe calling it a backup is better because that doesn't clash with "archive formats", such as tar.

3) How to restore a backup?

4) I'm strongly in favor of creating daily instead of weekly backups.

minhqdao commented 1 year ago

After I changed my email address, I received a new verification email for the new email address, which is great. However, the verification status of the new email address is never being checked, since I have access to the full functionality without having verified the new email address. That should be changed.

henilp105 commented 1 year ago

Hi @minhqdao , We are making weekly snapshots of the entire registry ( except user information) as required by T3 in Specifications and general registry requirements, This will involve obtaining a list of all packages and versions, and bulk downloading of packages , This is to support the functionality of Cloning of the registry and to allow the ability Mirror registry ( please refer: https://github.com/fortran-lang/registry/issues/2 ) .

Sure, We could easily keep that name as well .

It would be as simple as loading the tar into the mongodb.

I would totally agree for daily interval , i had set at a weekly interval due to resources limitation.

minhqdao commented 1 year ago

Has https://github.com/fortran-lang/registry/issues/40#issuecomment-1579851827 been implemented?

minhqdao commented 1 year ago

I think we should resolve all remaining issues/comments and then have this PR merged as soon as possible. Let's keep all future PRs small and restricted to single functionality changes or bug fixes only.

henilp105 commented 1 year ago

Thanks @minhqdao , I have patched it. I have implemented #40 but I have disable it just like the package validation functionality.

minhqdao commented 1 year ago

Thanks @minhqdao , I have patched it. I have implemented #40 but I have disable it just like the package validation functionality.

I still can't see a change after changing my email address. What should look out for?

Actually I think that the change of the email address should only succeed after having verified the new email address. The old email address would still be used until then. Imagine someone having an account with lots of uploaded packages already. Now the user wants to change the email address and introduces a typo by accident. The user will never receive a notification email and is forever logged out without the chance to ever get back in to change the email address again.

Also, could the email verification popup also receive a little user feedback while waiting for the server response? Maybe a loading indicator before Email id Successfully changed shows? I also think that Email successfully changed. is a more proper response.

henilp105 commented 1 year ago

Thanks @minhqdao , I think this would be a possible solution for the bug, if the user changes email with a typo then until the user is not logged out till then the user will be able to change the email (as many times as needed) and we can prompt user to verify the email after changing email. once he logs out them the user would have to verify the email to login back. sure, I would also add the loading to the modal.

minhqdao commented 1 year ago

Can we please have it like this:

Actually I think that the change of the email address should only succeed after having verified the new email address. The old email address would still be used until then. Imagine someone having an account with lots of uploaded packages already. Now the user wants to change the email address and introduces a typo by accident. The user will never receive a notification email and is forever logged out without the chance to ever get back in to change the email address again.

I think this is the easiest and safest way. They way you proposed, the user would still be in trouble when logged out after having changed to a wrong email address.

henilp105 commented 1 year ago

Sure @minhqdao , I will patch this.

henilp105 commented 1 year ago

I have patched it. Can you please review it @minhqdao ?

minhqdao commented 1 year ago

Loading indicator?

minhqdao commented 1 year ago

Nope, it doesn't work. I'm not even receiving a verification email anymore. Have you checked it yourself if it works?

henilp105 commented 1 year ago

sure @minhqdao , let me recheck it once again. I will ping you again for the review , thanks.

henilp105 commented 1 year ago

@minhqdao , I have tested It . can you please review it ?

minhqdao commented 1 year ago

Can't log into any of my accounts anymore. I created a new one, but the password wasn't registered by Safari's password manager, so I verified the mail and wanted to reset the password. Now it says "Please verify your email". But it is verified.

I don't feel like this is all working well. Please take your time to check all the authentication flows and make sure it all works well to the best of your knowledge before requesting reviews.

henilp105 commented 1 year ago

Sure @minhqdao , I have patched it and you would have to create a new account as I had to clear the entire db to add a new paramter to the users collection for the new unverified email, can you please review it again ?

Testable link: https://registry-phi.vercel.app/

minhqdao commented 1 year ago

Cannot change password. Still the old one is valid.

henilp105 commented 1 year ago

Thanks @minhqdao , good catch , I have patched this up.

minhqdao commented 1 year ago

Have you tested it yourself?

henilp105 commented 1 year ago

yes @minhqdao , I have tested.

minhqdao commented 1 year ago

"Change Password" still doesn't work for me.

henilp105 commented 1 year ago

@minhqdao I have fixed that , the password was being changed , just the notification on the user side had been disabled, I have enabled it. Can you please review it again ?

minhqdao commented 1 year ago

Oh, I see now that the UI doesn't show any error response, which had caused the problem.

If I attempt to change the password and the old password I entered wasn't correct, I do not get a response.

First off, and as mentioned many times in this PR:

1) Have a loading indicator that indicates that the request is currently being processed.

Then:

2) The problem here was that when I entered an incorrect old password, I do not get an error message. I, in fact, get no message at all, which might imply that everything worked normally.

henilp105 commented 1 year ago

@minhqdao Thanks for the review , i have fixed them. Can you please review it again ?

minhqdao commented 1 year ago

I see the code change but the input field still says "Email" only. I did clear the cache. Is it because it hasn't been deployed yet?

henilp105 commented 1 year ago

yes exactly, I have deployed it now. @minhqdao

minhqdao commented 1 year ago

Maybe you can set up an automatic deployment so this doesn't happen in the future.