Fixes #820: maintains project.archived value when their org is archived/unarchived

[laura-barluzzi]

Proposed changes in this pull request

This PR fixes #820 Already archived projects get unarchived when org is unarchived by:

• Removing the lines of code that manually set all project.archived=True when archiving an organization in organization/views/default.OrgArchiveView.archive()
• Adding organization__archived=True as an additional filter value for public projects in organization/views/default.ProjectList.get() so that all projects of archived organizations aren't displayed, despite their own .archived field setting.
• Changing the query in if is_admin: block using the OR query logic with Q objects.

filter(Q(archived=True) | Q(organization__archived=True))

This simply adds the archived projects of not-archived organizations and all projects of archived organizations.

• Adding the Archived red flag label next to the organization.name in both organization/project_wrapper.html and organization/project_list.html templates to better visualize the situation. This is mainly useful in case there are not-archived projects in an archived organization -- the missing project Archived red flag label is substituted by the one for their organization.
• Changing 2 assert statements in OrganizationUnarchiveTest and OrganizationArchiveTest to meet the new logic for which the archiving of projects doesn't take place.

When should this PR be merged

• When it is reviewed and approved.

Risks

• Low

Follow-up actions

• Double check other locations where the same logic could be applied -- e.g. are we showing archived organization projects to users who don't have permission for that somewhere else?
• Close issue Already archived projects get unarchived when org is unarchived Already archived projects get unarchived when org is unarchived #820

Checklist (for reviewing)

General

• Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
  • Review 1
  • Review 2
• Is the PR labeled correctly? It should have the migration label if a new migration is added.
  • Review 1
  • Review 2
• Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.
  • Review 1
  • Review 2

Functionality

• Are all requirements met? Compare implemented functionality with the requirements specification.
  • Review 1
  • Review 2
• Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.
  • Review 1
  • Review 2

Code

• Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
  • Review 1
  • Review 2
• Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
  • Review 1
  • Review 2
• Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.
  • Review 1
  • Review 2
• Is the code documented sufficiently? Large and complex classes, functions or methods must be annotated with comments following our code-style guidelines.
  • Review 1
  • Review 2
• Has the scalability of this change been evaluated?
  • Review 1
  • Review 2
• Is there a maintenance plan in place?
  • Review 1
  • Review 2

Tests

• Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
  • Review 1
  • Review 2
• If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
  • Review 1
  • Review 2
• If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?
  • Review 1
  • Review 2

Security

• Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
  • Review 1
  • Review 2
• Are all UI and API inputs run through forms or serializers?
  • Review 1
  • Review 2
• Are all external inputs validated and sanitized appropriately?
  • Review 1
  • Review 2
• Does all branching logic have a default case?
  • Review 1
  • Review 2
• Does this solution handle outliers and edge cases gracefully?
  • Review 1
  • Review 2
• Are all external communications secured and restricted to SSL?
  • Review 1
  • Review 2

Documentation

• Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
  • Review 1
  • Review 2
• Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
  • Review 1
  • Review 2
• Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.
  • Review 1
  • Review 2

[seav]

Hi, @laura-barluzzi, thanks for submitting this PR. I only have a few comments.

For the unit tests you modified, I think you should change the test name because the archiving no longer cascades to the project level. So a name like test_archive_cascade_to_projects is no longer true.

[seav]

If the organization has a logo, this archived label does not appear. (Admittedly, we can't upload org logos now, but this should still be considered.)

[laura-barluzzi]

Sure thing. I didn't notice. For now I move the label outside, so it appears also with logos 👍

[amplifi]

There's an issue with the approach here; the proposed solution listed in #820 is invalid. When an organization is archived, we should continue to automatically archive its projects for a few reasons. 1) The state of a project should be consistent throughout the platform. The project of an archived organization shouldn't have its archived flag set to false while not being displayed among active projects. The implementation here creates a third state between archived and active, that of unarchived project of an archived org, which adds unnecessary complexity. We don't want to create situations in which this third state now needs to be accounted for throughout the platform UI/API and microservices, and it's counterintuitive for users and developers alike. 2) It's logically inconsistent to have an active project of an inactive organization, so we should avoid creating this case. 3) Projects are filtered by archived flag to determine whether or not their search indexes are updated; this implementation would have us unnecessarily indexing projects whose data is no longer active. This isn't the only use case where having the option to filter by active projects is of value, so retaining the validity of this flag is important. 4) When unarchiving an organization, we don't automatically want to mark every project as active. Unarchiving should be an intentional process. If an org has multiple projects, an admin may wish to only unarchive a subset of those. From a privacy and security perspective, unarchiving a project can make its data publicly available (for whatever data was public before archival). It should be a manual step for an admin to knowingly choose to make their project data available again, on a per-project basis.

The issue in #820 should be resolved by ensuring that a project's archived flag is set to True when archiving its organization, and that a project's flag is not modified (remains True) when its organization is unarchived.

[laura-barluzzi]

@amplifi thanks for your review. I went down this road mainly because the #820 issue states:

Possible solution
Don't archive the projects when an org is archived. But treat projects in an archived org as if they were archived as well regardless of the project's actual archived state.

I guess I interpreted this in my own way :) This said, I have in mind another potential solution:

• Add Project.archived_by_org field: this field is by default False.
• Whenever we archive an organization, we manually archive the not-already-archived projects and we also set archived_by_org=True to these projects.
• Whenever we unarchive an organization, for each project we check if project.archived_by_org=True. If it was archived by the organization archiving, we unarchive the project. Otherwise we keep it as it is, and we don't do anything.

In other words:

1. We cascade the organization archiving to its projects only when the projects aren't already archived themselves. If they aren't archived, we archive them and we set proj.archived_by_org=True;
2. We cascade the organization unarchiving to its projects only when the projects were archived via the cascading of the organization archiving. Therefore, when proj.archived_by_org=True.

Today I make this change, it should be fairly easy and quick to do.

[laura-barluzzi]

I close this and open a new one.