Do not allow login to inactive members -- solved

[franpb14]

In this branch I focused on solving the problem related to the issue #472. The solution that I suggest is the following: an user trying to access an organization in which he/she was deactivated will be redirected to a view that shows a warning message as well as a table with a list of the organzations (and all the information related to those organizations). The behavior of offers and inquiries is the same as for that of non-authenticated users, with the exception of those offers/inquiries coming from an organization in which they are active.

I additionnaly changed the "set_current_organization" so those users who had been deactivated in their first organization do not have to see the error message every time they log in.

Closes #472

[markets]

Hi @franpb14 👋🏼 and very welcome to TimeOverflow! 😄

As this is your 1st pull request here, let me add some general comments with tips, good practices, style ...

• First of all, this branch seems a bit outdated with develop. According to the branches page: https://github.com/coopdevs/timeoverflow/branches you are 20 commits behind develop branch (our base branch). Before pushing a new branch, it's a very good practice to rebase/merge the latest changes from the base branch (otherwise you may be missing important changes, from a technical POV, but also from a product view).
• Instead of mixing branches with bugfixes and features, we should focus on small problems, since they simplify the diff (-> the review), the testing, and they are easier to reason about. So, to solve Do not allow login to inactive members #472 we don't need all these changes and behavior changes. Let's focus on small pieces 🙏🏼 ! At least during this training/onboarding stage.
• Code style: there are some issues about indentation (we use 2 spaces for HTML too), empty lines, comments in spanish... that should be addressed first. You can use Rubocop locally to be sure your new code matches our current code style.
• In general, having logic or sql queries in the views, is a bad practice and we should try to avoid it always (it makes testing/maintenance really hard).
• We should also take care with code coverage, according to CodeClimate we are losing 7% of coverage in this branch
• You can use "magic" comments (https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) to automatically close issues when merging a PR

To move forward with this branch, IMO, we should just fix the problem described in #472. Let's start with small changes, step by step, it will be easier for all the team to give feedback, perform the code review, write all necessary tests...

Thanks a lot for working on this and welcome to TimeOverflow Fran (and sorry for this long comment 😜 )

[franpb14]

Thanks @markets for the review, I expected something like this, but I have some doubts:

• To finish with this issue, I need to make a branch to fix it, another one to test it and another one to change the behavior, right?
• I don't understand why the coverage dropped because I tested my methods and, obviusly, I did not delete any tests. Maybe it's the outdated?
• When I start a larger task, if I don't test it on the same branch, the coverage will go down.
  I'm looking forward to make some useful code for timeoverflow :)

[markets]

To finish with this issue, I need to make a branch to fix it, another one to test it and another one to change the behavior, right?

No, no, tests and code should be in the same branch, but mixing small fixes with behavior changes (which can have more implications) it's a bad idea in general. Small branches are the best! In that case, if we want to avoid the login for non-active users, we can just (for example) redirect back with a flash message "You can't login to this organization", with no need to implement new pages and workflows.

I don't understand why the coverage dropped because I tested my methods and, obviusly, I did not delete any tests. Maybe it's the outdated?

Yep, this is right, probably it's just a matter of updating the branch.

---

Best,
Marc

[franpb14]

Maybe I'm wrong but I think your proposed solution will create a critical bug. Without change the behaviour of login or without the new page, the users who were deactivated from the first organisation will not be able to login even if they are active in another organisation.

[markets]

Ah got it! you're right about that point 👍🏼 , now I understand better why you took this approach 😄 If @sseerrggii (the product owner) likes this solution, we can focus only on the "technical" aspects of this branch. For PRs that contains UI changes, it's a good idea to add some screenshots (or gif or video) in the description 👌🏼

[franpb14]

In the following video we can see the changes.
First, the user logs in and sees the error because he was deactivated from the first organisation, he can see the bank information with a modal, and then we can see that the behaviour for offers/demands is the same as for unidentified and finally we can see that he can log in from the view and what happens when he tries to switch to an organisation from which he was deactivated.

Issue472.mp4

I saw that travis failed but locally the tests run correctly and the log on Travis are different for each build. Why? Maybe it's a Travis fault?

[markets]

Hi @franpb14 that seems a flaky test, the PR build is passing. I tried to restart the build in Travis, but I got an error:

Oh no! An error occurred. The build could not be restarted.

So, no worries about that point (will see in the next push), the build is also passing for me locally. Probably it's an order dependent test (which means we have an issue in our specs 😄 ). In rspec, the order can be reproduced the via the --seed argument: rspec --seed 1234. The seed is printed on each run, in the terminal ("Randomized with seed 1234").

I'd add some extra comments/feedback but now seems much better in general 👌🏼 Thanks for reviewing the comments.

[franpb14]

@markets Thank you very much for taking your time correcting it, I really appreciate it. I did a new push implementing feedback but the current_organization can't be nil easily because in "application_controller" the organisation is set for each action so that when the user enters, for example, /offers current_organisation would be set again. After talking to @sseerrggii we decided not to do that for this issue.

[markets]

@franpb14 @sseerrggii I pushed one commit with some minor tweaks (fa5c885). This is ready for testing from my side ✔️ Good job @franpb14 👍🏼

ℹ️ @sseerrggii we'll need to add 4 new keys in Locale

[sseerrggii]

Works fine! Thanks @franpb14 and @markets 💪