cribeiro84 / azure-devops-pull-request-hub

Azure DevOps Pull Request Manager Hub
MIT License
59 stars 35 forks source link

Changing sorting causes filters to be ignored #251

Open Xerillio opened 1 year ago

Xerillio commented 1 year ago

Describe the bug Changing the sorting after adding filters causes pull requests that should have been filtered out to show up again.

To Reproduce Steps to reproduce the behavior:

  1. Create two or more pull requests (e.g. with different source branches)
  2. Go to the PRMH and make sure both PRs show up
  3. Add a filter to filter out one of the PRs (e.g. choose only one source branch)
  4. Confirm that the expected PR shows in the table
  5. Change the sorting directing on the "When" column
  6. Now both PRs are shown in the list again

Expected behavior Changing sorting should not affect which PRs are shown in the table - only the order.

Desktop (please complete the following information):

Xerillio commented 1 year ago

@cribeiro84 I've been trying without success to figure out how this.state.pullRequests and this.pullRequestItemProvider are connected and what the purpose of each is. I'm guessing one is for rendering/sub-component props and the other is the underlying list (i.e. cached data source). Do you remember if there's a reason it's not all being maintained in one list in the component's state?

The following is a dirty fix for this issue, but I think I'd need a better understanding of this.state.pullRequests vs. this.pullRequestItemProvider to come up with something better.

    const sortingBehavior = new ColumnSorting<
      PullRequestModel.PullRequestModel
    >((columnIndex: number, proposedSortOrder: SortOrder) => {
      this.pullRequestItemProvider.splice(
        0,
        this.pullRequestItemProvider.length,
        ...sortItems<PullRequestModel.PullRequestModel>(
          columnIndex,
          proposedSortOrder,
          this.sortFunctions,
          this.columns,
-         pullRequests
+         this.pullRequestItemProvider.value as PullRequestModel.PullRequestModel[]
        )
      );
      this.setState({ sortOrder: proposedSortOrder });
    });

Link to the first line in the snippet

cribeiro84 commented 1 year ago

@Xerillio thanks for brining this up! I will be honest with you, the code itself is not the best one I could develop because it was my first time writing an ADO extension and a React App :-P

My main goal of this project would be to re-write it from scratch removing all the workarounds I had to do but I am struggling to find free time to do it (my little son loves to play till late night).

Answering your question, the this.state.pullRequests is all the PRs that were fetched, it's "cached" and the filtered on the fly because most of the APIs provided by ADO does not have filters and also to avoid chatty API calls to the server. If I am not mistaken, the pullRequestItemProvider is related to the applied filters.

Xerillio commented 1 year ago

@cribeiro84 Completely understandable 😊 ADO extensions are also completely new to me and only limited experience with React. I'll try to give it a go restructuring it by keeping the cached list separate from the Reach component (PullRequestsTab) so the component only maintains the shown/filtered list. If I succeed, I'll submit a PR.

Happy new year!