Skip to content

Conversation

@PrakashDurlabhji
Copy link
Contributor

No description provided.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thank you for update @PrakashDurlabhji

I can see delete button now, but it doesn't work for me:
https://monosnap.com/file/nZt8beLqnXge5lNP1Vowvi3ihBKrse

Please, submit a demo video on how it works for you.

Thank you.

@maxceem
Copy link
Collaborator

maxceem commented Apr 17, 2019

@PrakashDurlabhji just in case would like to remind about this PR as the Bug Bash challenge in review phase now, which will be finished in 1.5 days.

@PrakashDurlabhji
Copy link
Contributor Author

@maxceem which api needs to be used?
removeProjectAttachment?

or

updateProjectAttachement with proper payload?

@maxceem
Copy link
Collaborator

maxceem commented Apr 17, 2019

@PrakashDurlabhji removeProjectAttachment, I think the issue is because you call with idx but I guess it should be called with attachmentId

@maxceem
Copy link
Collaborator

maxceem commented Apr 17, 2019

@PrakashDurlabhji there are also some conflicts with other changes has been done during this Bug Bash, please kindly resolve them before I can accept this PR.

@PrakashDurlabhji
Copy link
Contributor Author

@maxceem is edit attachement working proper already?

I dont think edit attachement is working as even it uses idx,

Also there is no attachementId property in filelinks object, hence will implement same logic as links delete which splices the element at idx

@maxceem
Copy link
Collaborator

maxceem commented Apr 17, 2019

is edit attachement working proper already?

it was broken and it has been fixed in another issue #2938

I dont think edit attachement is working as even it uses idx,

right, it didn't work with idx

Also there is no attachementId property in filelinks object, hence will implement same logic as links delete which splices the element at idx

please, get the latest commits in the cf16 branch, and check how the issue #2938 has been fixed in this PR #2942

I think in that issue attachementId has been added so you can reuse it in your submission.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks @PrakashDurlabhji

Deleting works now, but there are several issues:

  1. Delete button should be shown following the same conditions when edit button is shown. But currently if I upload two file with different users I can see delete button for this file which has been uploaded by another user, but it shouldn't:

    image

  2. When we click delete button we show confirmation dialog for removing file, but the text says about removing a link:

    image
    For the files, this dialog should have text about file instead of link. While for links it should staty as it is.

  3. removeAttachment function has an argument called idx, but it's not an index but attachmentId https://github.com/appirio-tech/connect-app/pull/2959/files#diff-1bd3d27391bef49dfabf369856ff817fR134

    Please, rename it accordingly.

Thank you.

@PrakashDurlabhji
Copy link
Contributor Author

@maxceem MR updated with all points

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thank you @PrakashDurlabhji

There are few issues:
in this line https://github.com/appirio-tech/connect-app/pull/2959/files#diff-1bd3d27391bef49dfabf369856ff817fR351 laodProjectMessages is undefined - I think it happened during resolving conflicts.

After we remove a file, uploading a new file cannot be completed.

Will fix them myself. Other than that PR is good and accepted.

@maxceem maxceem merged commit 877b830 into topcoder-archive:cf16 Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants