Skip to content

New Feature : conflicting claims #279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

New Feature : conflicting claims #279

wants to merge 1 commit into from

Conversation

YashKumarVerma
Copy link
Contributor

@YashKumarVerma YashKumarVerma commented May 18, 2020 •
edited
Loading

Fixes #260
Ref #258

  • Shows a list of conflicting claims made by various users in the sidebar
  • Provision to match URLs who differ only in the protocol, trailing slashes, ports, hashes, query strings etc
  • Inline documentation added for codebase edits
  • All errors inside route handled

Preview

  • Raw data used to build page
    screencapture-localhost-3232-claims-2-2020-05-18-12_40_48

  • When there are no conflicts
    Screenshot from 2020-05-18 13-38-02

  • When conflicts arise
    Screenshot from 2020-05-18 13-38-14

@YashKumarVerma YashKumarVerma mentioned this pull request May 18, 2020
Open
thenamankumar
thenamankumar suggested changes May 18, 2020
View reviewed changes
utils/datautils.js Outdated
@@ -48,6 +48,18 @@ function getClaimById(claimId) {
return db.Claim.findById(claimId)
}

function getConflictingClaimByPr(pullRequestId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the correct logic. just checking the presence of id does not decide the id is for pr or issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i named the parameter wrongly in this one.
for a the url
https://github.com/coding-blocks/boss/pull/279
this is the value of the parameter
coding-blocks/boss/pull/279
and not only 279

Copy link
Contributor Author

@YashKumarVerma YashKumarVerma May 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

The caller function does not search for id only, it searches for full generic URL :
boss/pull/279

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks the unique part of the URL, which is of the type user/repository/pull/id. The above confusion arose due to improper variable naming, which I've fixed now.

routes/root.js
let response, conflicts

/**
* the database has internal checks for duplicate items in the pull request column
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not cover all the cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please mention which type of case is not handled

@YashKumarVerma YashKumarVerma mentioned this pull request May 18, 2020
Closed
@YashKumarVerma
Copy link
Contributor Author

YashKumarVerma commented May 19, 2020 •
edited
Loading

@hereisnaman, @championswimmer waiting for review

@thenamankumar
Copy link
Contributor

@hereisnaman, @championswimmer waiting for review

Reviews usually take from 1-3 days. You can start working on other issues.

@YashKumarVerma
Copy link
Contributor Author

Awaiting review.

@YashKumarVerma
Copy link
Contributor Author

YashKumarVerma commented May 24, 2020 •
edited
Loading

Requesting someone to carry it forward

@YashKumarVerma
Copy link
Contributor Author

A few thigns to mention:

  • if only the number is matched using a wildcard, then issue 10 will show conflicts with 100, 101, 102...
  • if it will be assumed that the issue / pr id will be followed by a slash or terminate immediately, it's also a wrong lead, as URLs ending with hashes are also submitted

Working on a solution of the above problems.

@thenamankumar
Copy link
Contributor

A few thigns to mention:

* if only the number is matched using a wildcard, then issue 10 will show conflicts with 100, 101, 102...

* if it will be assumed that the issue / pr id will be followed by a slash or terminate immediately, it's also a wrong lead, as URLs ending with hashes are also submitted

Working on a solution of the above problems.

What is the use case behind having urls with hashes? Can't we clean them in the add claim form?

@YashKumarVerma
Copy link
Contributor Author

A few thigns to mention:

* if only the number is matched using a wildcard, then issue 10 will show conflicts with 100, 101, 102...

* if it will be assumed that the issue / pr id will be followed by a slash or terminate immediately, it's also a wrong lead, as URLs ending with hashes are also submitted

Working on a solution of the above problems.

What is the use case behind having urls with hashes? Can't we clean them in the add claim form?

We should surely add checks in the form where new claim is added, but the problem is, We already have entries like this.
image
image

So either we update the entries like these, or we accommodate things like this in the conflicts code. In any of these cases, we still need to add checks in the input form

@thenamankumar
Copy link
Contributor

thenamankumar commented May 30, 2020 •
edited
Loading

@YashKumarVerma can you first create a PR with the following:

  • in add claim api, clean the urls, removing all the hashes, query params and add trailing slash.
  • write a script to transform the current items for the same.

I'll give you separate points for that.

@YashKumarVerma
Copy link
Contributor Author

YashKumarVerma commented May 30, 2020 •
edited
Loading

@YashKumarVerma can you first create a PR with the following:

  • in add claim api, clean the urls, removing all the hashes, query params and add trailing slash.
  • write a script to transform the current items for the same.

I'll give you separate points for that.

@hereisnaman I wrote the script but there's one issue we need to tackle.
The pullUrl column is set as unique, so if there exist entries with and without trailing slash, it will throw error and not let us update it.

An alternative approach can be that we

  • do not fix the old claims (as removing the slash and # would cause conflicts in db as the column is set to unique in the schema and ideally, we should not change that)
  • we check new claims that are made, and make sure that the above point is not repeated in new claims.
  • in the conflicting claims page, we write the code to handle the cases like these (I've found a workaround for it)

@YashKumarVerma YashKumarVerma mentioned this pull request May 30, 2020
Open
@YashKumarVerma
Copy link
Contributor Author

YashKumarVerma commented May 30, 2020 •
edited
Loading

With #390 sent, now we can create a more robust conflict view.
Reasons:

  • There are multiple PRs sent for the same issue. we need to list them.
  • Shows number of PRs for single issue submitted on BOSS portal
  • image
  • There are
    • 100 claims which have the same URL submitted for both handles.
    • 140 claims which have different URLs submitted

admins must have a complete view of where all were the issues and pull requests submitted

@YashKumarVerma YashKumarVerma mentioned this pull request May 30, 2020
Closed
@YashKumarVerma
Copy link
Contributor Author

Carried forward in #392

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thenamankumar thenamankumar Awaiting requested review from thenamankumar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add conflicting claims section on accept claims page
2 participants
@YashKumarVerma @thenamankumar